-
Notifications
You must be signed in to change notification settings - Fork 2k
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
EventHub stream provider partition checkpoint corruption fix #1760
Conversation
87334ad
to
1f8adad
Compare
@jason-bragg , should that be merged? Bug fix, right? |
@gabikliot, Yes, this is a bug fix. I think it's ready for merge, but I was out a couple weeks, so I don't think anyone reviewed this yet. If you've looked at it, that's good enough for me. :) |
Got you. I will leave it up for @jdom . |
@@ -11,6 +12,7 @@ namespace Orleans.ServiceBus.Providers | |||
/// </summary> | |||
public static class EventDataExtensions | |||
{ | |||
private static string[] SkipProperties = { "Offset", "SequenceNumber", "EnqueuedTimeUtc", "StreamNamespace" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use strongly typed versions, such us:
SkipProperties = { nameof(EventData.Offset), nameof(EventData.SequenceNumber), nameof(EventData.EnqueuedTimeUtc), EventDataExtensions.EventDataPropertyStreamNamespaceKey };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I used EventData
, but not sure whether this was the type you were using
LGTM 👍 |
I'll update to use strongly typed property names, but lets hold this until after the ServiceBus 3.2.2 update, which I'm working on now in another branch. |
…ed due to a change to the way the offset was being stored in the cache. Offset is now stored consistently and in an accessible way for the checkpointing. This bug was masked by timing issues in the tests, so test cleanup and timeouts have been modified.
1f8adad
to
6fc24fd
Compare
Updated. Got to go |
@dotnet-bot test this please |
The EventHub stream provider partition checkpoints were being corrupted due to a change to the way the offset was being stored in the cache.
Offset is now stored consistently and in an accessible way for the checkpointing.
This bug was masked by timing issues in the tests, so test cleanup and timeouts have been modified.