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

sharding snapshot & journal cleanup #2703

Conversation

zbynek001
Copy link
Contributor

@zbynek001 zbynek001 force-pushed the sharding-delete-messages-after-snapshot branch from 77aeb4a to dc0d62c Compare May 26, 2017 01:07
@zbynek001 zbynek001 changed the title [WIP] sharding snapshot & journal cleanup sharding snapshot & journal cleanup May 26, 2017
Copy link
Contributor

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good, but string format for new logs is incorrect (akka on jvm uses {} but akka.net requires argument indexing like {0}).

}
break;
case SaveSnapshotFailure m:
Log.Warning("PersistentShard snapshot failure: {}", m.Cause.Message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In .NET logging we must provide index for arguments. So {} itself will fail, you need to use {0}, {1} and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, completely overlooked that. Fixed

Log.Debug("PersistentShard snapshots matching {0} deleted successfully", m.Criteria);
break;
case DeleteSnapshotsFailure m:
Log.Warning("PersistentShard snapshots matching {0} deletion falure: {1}", m.Criteria, m.Cause.Message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be failure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@alexvaluyskiy
Copy link
Contributor

alexvaluyskiy commented May 26, 2017

@zbynek001 You have also implemented this issue
akka/akka#20038
Because we was not handling snapshots before in PersistentShard

@zbynek001
Copy link
Contributor Author

I think that's obsolete code.
SaveSnapshotSuccess & SaveSnapshotFailure is already handled inside PersistentShard

@zbynek001 zbynek001 force-pushed the sharding-delete-messages-after-snapshot branch from 96ff40c to 32deffd Compare May 26, 2017 14:31
@Aaronontheweb Aaronontheweb merged commit 1f988d7 into akkadotnet:v1.3 May 30, 2017
Arkatufus pushed a commit to Arkatufus/akka.net that referenced this pull request Jun 6, 2017
Aaronontheweb pushed a commit to Aaronontheweb/akka.net that referenced this pull request Jun 28, 2017
@zbynek001 zbynek001 deleted the sharding-delete-messages-after-snapshot branch June 29, 2017 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants