Enable legacy checkpoint reading in WebJobs #17392
Conversation
ghost
added
the
| @@ -344,7 +344,8 @@ await foreach (BlobItem blob in ContainerClient.GetBlobsAsync(traits: BlobTraits | |||
|
|
|||
| async Task<List<EventProcessorCheckpoint>> listLegacyCheckpointsAsync(List<EventProcessorCheckpoint> existingCheckpoints, CancellationToken listCheckpointsToken) | |||
| { | |||
| var legacyPrefix = string.Format(CultureInfo.InvariantCulture, LegacyCheckpointPrefix, fullyQualifiedNamespace.ToLowerInvariant(), eventHubName.ToLowerInvariant(), consumerGroup.ToLowerInvariant()); | |||
| // Legacy checkpoints are not normalized to lowercase | |||
| var legacyPrefix = string.Format(CultureInfo.InvariantCulture, LegacyCheckpointPrefix, fullyQualifiedNamespace, eventHubName, consumerGroup); | |||
There was a problem hiding this comment.
Without this change, are the legacy checkpoints skipped? Do we have a test for this?
There was a problem hiding this comment.
That's an interesting question... I think that Storage is case-sensitive for naming, but I'm not 100% sure.
There was a problem hiding this comment.
Yes, I changed the existing tests to use the expected casing both to T1 and T2 checkpoints. Storage is case-sensitive.
| @@ -60,7 +60,7 @@ public async Task ListCheckpointsAsync_LogsOnInvalidCheckpoints() | |||
| { | |||
| Page<BlobItem>.FromValues(new[] | |||
| { | |||
| BlobsModelFactory.BlobItem("testnamespace/testeventhubname/testconsumergroup/checkpoint/0", false, BlobsModelFactory.BlobItemProperties(false), metadata: new Dictionary<string, string>()) | |||
| BlobsModelFactory.BlobItem("testnamespace/testeventhubname/testconsumergroup/checkpoint/0", false, BlobsModelFactory.BlobItemProperties(false, contentLength: 0), metadata: new Dictionary<string, string>()) | |||
There was a problem hiding this comment.
Why do we pass contentLength of 0?
There was a problem hiding this comment.
There is no content for a checkpoint blob; its a zero-byte file. The data exists entirely in the metadata. I'm not sure why the text is explicitly setting this, as no logic relies on it, but it does mirror what is expected for the actual service return.
There was a problem hiding this comment.
There was a problem hiding this comment.
ah, fair enough. I was thinking in terms of the T2 checkpoints. Makes sense in the legacy context.
| @@ -31,7 +31,7 @@ protected BlobsCheckpointStore() | |||
| public BlobsCheckpointStore(BlobContainerClient blobContainerClient, | |||
| EventHubsRetryPolicy retryPolicy, | |||
| string functionId, | |||
| ILogger logger): this(blobContainerClient, retryPolicy) | |||
| ILogger logger): this(blobContainerClient, retryPolicy, readLegacyCheckpoints: true) | |||
There was a problem hiding this comment.
Should we consider renaming to something like initializeWithLegacyCheckpoints to reflect that the intent is to read them once and then replace them?
|
Hello @pakrym! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
Fixes: Azure#17034 Also include name casing in tests.
Fixes: #17034
Also include name casing in tests.