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

StandByFeedIterator breath first #1121

Merged
merged 2 commits into from
Jan 10, 2020
Merged

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Dec 28, 2019

Pull Request Template

Description

Currently the StandByFeedIterator will only move to the next partition if the current one has no more changes.

In scenarios where all partitions are receiving constant changes, changes from other partitions might get delayed.

With this change, the StandByFeedIterator will move to the next range upon receiving a Change Feed response, so the next ReadNextAsync will read from the next range.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Closing issues

This PR closes #1117

@ealsur ealsur added feature-request New feature or request ChangeFeed labels Dec 28, 2019
@ealsur ealsur self-assigned this Dec 28, 2019
@ealsur ealsur added this to In progress in Cosmos DB SDK team via automation Dec 28, 2019
@kirankumarkolli
Copy link
Member

Is breadth ask for even a single request response or it for multiple iterations?

@ealsur
Copy link
Member Author

ealsur commented Jan 2, 2020

Not sure I understand the question. The current model will stay on the same Partition until that partition has no more changes (304). The requested change will move to the next Partition on each request.

So in the case of Partitions A, B, and C, where A has 100 changes, B has 50, and C has 50, on the current model we would (with a MaxItemCount of 10):

  1. Read A 10 times (10 calls to ReadNextAsync)
  2. Read B 5 times (next 5 calls to ReadNextAsync)
  3. Read C 5 times (next 5 calls to ReadNextAsync)

The problem is that, if while we are reading A, more changes keep pouring over A, we will never move to other Partitions and the changes happening on them will keep lagging behind.

With the PR change, we will continuously circle through Partitions on each ReadNextAsync to get 1 set of changes from each Partition, so on the same scenario:

  1. Read 10 items from A (1 ReadNextAsync)
  2. Read 10 items from B (1 ReadNextAsync)
  3. Read 10 items from C (1 ReadNextAsync)

And back to 1. It's the same number of reads to get all changes, just breath first. And if new changes keep pouring over partition A, we won't lock on partition A and let B and C stale.

@kirankumarkolli
Copy link
Member

Assuming compute is okey with it as default behavior.
/cc: @jasontho-ms

Cosmos DB SDK team automation moved this from In progress to Reviewer approved Jan 7, 2020
@@ -62,6 +62,9 @@ public override async Task<ResponseMessage> ReadNextAsync(CancellationToken canc
do
{
(currentKeyRangeId, response) = await this.ReadNextInternalAsync(cancellationToken);
// Read only one range at a time - Breath first
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: New line before?

Copy link
Contributor

Choose a reason for hiding this comment

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

and nit: breadth with a 'd'

/// </summary>
[TestMethod]
[Timeout(30000)]
public async Task StandByFeedIterator_BreathFirst()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's breadth, with a 'd'

@kirankumarkolli kirankumarkolli merged commit dec96cf into master Jan 10, 2020
Cosmos DB SDK team automation moved this from Reviewer approved to Done Jan 10, 2020
@kirankumarkolli kirankumarkolli deleted the users/ealsur/breathfirst branch January 10, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangeFeed feature-request New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

StandByFeedIterator breath first
3 participants