Retrieve checkpoints one by one #18127
Conversation
ghost
added
the
jsquire
left a comment
jsquire
left a comment
There was a problem hiding this comment.
Looks solid. I don't see anything functional for feedback, mostly just formatting nits and doc thoughts.
You're welcome to ignore the formatting/docs feedback to advance this. I left it as much as notes for myself; I'm happy to loop back around after this closes out and do the pedantic nit-picky things.
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/BlobCheckpointStore/BlobsCheckpointStore.cs
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/BlobCheckpointStore/BlobsCheckpointStore.cs
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/BlobCheckpointStore/BlobsCheckpointStore.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/BlobCheckpointStore/BlobsCheckpointStore.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/Processor/StorageManager.cs
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/Processor/StorageManager.cs
Show resolved
Hide resolved
| /// starting location set. | ||
| /// </remarks> | ||
| /// | ||
| protected abstract Task<IEnumerable<EventProcessorCheckpoint>> ListCheckpointsAsync(CancellationToken cancellationToken); |
There was a problem hiding this comment.
Do we want to tag this as [EditorBrowsable(false)] and possibly be a bit stronger and point them at GetCheckpointAsync in the <remarks> and potentially an [Obsolete] ?
The interplay between the pure abstract here and the virtual for GetCheckpointAsync may make this tricky.... it may be that non-browsable with a remark is the best we can do.... thoughts?
There was a problem hiding this comment.
[EditorBrowsable(false)] won't help, whoever inherits from EventProcessor would need to implement this method. I would also not rush to obsolete it because that would cause a compiler error for people with WarnAsError and we might realize we need ListCheckpointsAsync in some scenarios.
There was a problem hiding this comment.
Added a remark that overriding both is recommended.
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor.cs
Outdated
Show resolved
Hide resolved
pakrym
requested review from
KrzysztofCwalina,
serkantkaraca and
tg-msft
as code owners
|
Ported all tests from ListCheckpoint |
| } | ||
|
|
||
| /// <summary> | ||
| /// Indicates that an unhandled exception was encountered while retrieving a list of checkpoints. |
There was a problem hiding this comment.
Is this retrieving a list or a single checkpoint?
| } | ||
|
|
||
| /// <summary> | ||
| /// Indicates that an attempt to retrieve a list of checkpoints has completed. |
There was a problem hiding this comment.
Isn't this a single checkpoint?
| /// event processor instance, so that processing for a given partition can be properly initialized. | ||
| /// </summary> | ||
| /// | ||
| /// <param name="partitionId"></param> |
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs/src/Primitives/EventProcessor.cs
Outdated
Show resolved
Hide resolved
sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/Processor/StorageManager.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jesse Squire <[email protected]>
4 participants
A prototype to reduce the amount of requests we make to get checkpoints.