Internal ReadFeed: Adds pagination library adoption #1947
Conversation
bchong95
requested review from
ealsur,
FabianMeiswinkel,
j82w,
khdang,
kirankumarkolli,
kirillg and
sboshra
as code owners
| @@ -83,24 +84,40 @@ public DocumentContainer(IMonadicDocumentContainer monadicDocumentContainer) | |||
| cancellationToken), | |||
| cancellationToken); | |||
|
|
|||
| public Task<TryCatch<DocumentContainerPage>> MonadicReadFeedAsync( | |||
| public Task<TryCatch<ReadFeedPage>> MonadicReadFeedAsync( | |||
There was a problem hiding this comment.
Please run the MockedItemBenchmark.ReadFeed benchmark
Microsoft.Azure.Cosmos/src/Pagination/NetworkAttachedDocumentContainer.cs
Show resolved
Hide resolved
| @@ -190,6 +191,7 @@ public async Task ReadFeedIteratorCore_OfT_ReadAll_StopResume() | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| [Ignore] | |||
There was a problem hiding this comment.
Why are these tests being ignored?
| if ((overlappingRanges == null) || (overlappingRanges.Count != 1)) | ||
| { | ||
| // Simulate a split exception, since we don't have a partition key range id to route to. | ||
| CosmosException goneException = new CosmosException( |
There was a problem hiding this comment.
Is this to force the cache refresh?
There was a problem hiding this comment.
Should we instead force a cache refresh here? Instead of throwing the exception up and hoping it gets caught and handled?
| { | ||
| if (!(readFeedState.ContinuationToken is CosmosNull)) | ||
| { | ||
| request.Headers.ContinuationToken = (readFeedState.ContinuationToken as CosmosString).Value; |
There was a problem hiding this comment.
Would it make sense to guard against any potential nullref by checking if the continuation is a CosmosString in the upper if?
if (!(readFeedState.ContinuationToken is CosmosNull) && readFeedState.Continuation is CosmosString continuation)
There was a problem hiding this comment.
| continuation: continuationToken, | ||
| feedRangeInternal: feedRange, | ||
| options: requestOptions); | ||
| CosmosDiagnosticsContext readFeedDiagnostics = CosmosDiagnosticsContext.Create(requestOptions); |
| @@ -49,6 +49,7 @@ public async Task Cleanup() | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| [Ignore] | |||
There was a problem hiding this comment.
This test was to check that we could parallelize ReadFeed through FeedRanges, why was it ignored? Are we not allowing that scenario now?
| @@ -351,19 +384,6 @@ await feedIterator.ReadNextAsync(this.cancellationToken)) | |||
| Assert.AreEqual(batchSize, totalCount); | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| public async Task CannotMixTokensFromOtherContainers() | |||
There was a problem hiding this comment.
Why are we dropping the check for Rid? It would avoid users accidentally using a continuation that was not generated for the collection and getting incorrect results
| Assert.AreEqual(Documents.Routing.PartitionKeyInternal.MaximumExclusiveEffectivePartitionKey, feedTokenEPKRange.Range.Max); | ||
| Assert.AreEqual(continuation, feedRangeCompositeContinuation.CompositeContinuationTokens.Peek().Token); | ||
| Assert.IsFalse(feedRangeCompositeContinuation.IsDone); | ||
| Assert.AreEqual(numItems, count); |
There was a problem hiding this comment.
This is just checking that the iterator can handle splits, not that the handling was not done through the pipeline (which was the original intent of the test before). To check that no handler was doing the split handling and that it was correctly handled on the iterator (so it checked the number of transport calls)
|
|
||
| internal static class StreamExtensions | ||
| { | ||
| public static string ReadAsString(this Stream stream) |
|
Closing due to in-activity, pease feel free to re-open. |
Internal ReadFeed: Adds pagination library adoption
Description
Adds adoption of the pagination to finish unifying the feed story.
Related PRs:
From here on out all feed operations should call into a common library, which makes wiring things very easy.