Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqliteSnapshotStore is not getting table name from config #4079

Open

Conversation

@maratoss
Copy link

maratoss commented Dec 3, 2019

Table name was hardcoded. Seems that is breaking changes :(

@Aaronontheweb

This comment has been minimized.

Copy link
Member

Aaronontheweb commented Dec 3, 2019

Should close #4080

Copy link
Member

Aaronontheweb left a comment

Looks like you have some test failures - one of the backwards compat tests fails. Is the default value in HOCON equivalent to the previously hard-coded default value? If not, then this is a breaking change - fix it by making sure the default HOCON value == hard coded value from before.

@maratoss maratoss force-pushed the maratoss:bugfix/sqlite-is-no-getting-tablename-from-cfg branch from 98f75b9 to 1210dfb Dec 4, 2019
@maratoss maratoss requested a review from Aaronontheweb Dec 4, 2019
@Aaronontheweb

This comment has been minimized.

Copy link
Member

Aaronontheweb commented Dec 4, 2019

Tests look good - will review soon

/// <param name="connection">sqlite connection</param>
/// <param name="configuration">sqlite query configuration</param>
/// <returns>Returns true if has the conflict</returns>
public async Task<bool> Resolve(SqliteConnection connection, SqliteQueryConfiguration configuration)

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Dec 5, 2019

Member

... Why is this class necessary? I don't understand the need for the rest of the changes in this PR. I thought it was just a HOCON fallback?

This comment has been minimized.

Copy link
@maratoss

maratoss Dec 7, 2019

Author

Did not get the point with HOCON fallback :)

The idea was to do not get db corrupted for people who upgrade akka library.
This fix gets akka still use old hardcoded table 'snapshot' despite of 'table-name' setting value.

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Dec 10, 2019

Member

I'll take another look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.