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

Internal ChangeFeed: Adds adoption of pagination library #1933

Merged
merged 125 commits into from
Oct 20, 2020

Conversation

bchong95
Copy link
Contributor

@bchong95 bchong95 commented Oct 13, 2020

Internal ChangeFeed: Adds adoption of pagination library

Description

This PR adds the adoption of the new pagination library to ChangeFeed. This will enable us to have a single common code base for pagination across partitions, which has split proofing and the ability to simulate the service fabric emulator.

This is done by adding CrossPartitionChangeFeedAsyncEnumerator which implements IAsyncEnumerator<TryCatch<ChangeFeedPage>>. ChangeFeedIteratorCore now is just a wrapper around this class to provide API compact with FeedIterator internal.

While I was making this change I added a laundry list of improvements:

  • Added ChangeFeedException, so that we have a base type that a visitor can convert to the proper CosmosException. This is in preparation for this enumerator to run in compute.
  • Refactored the request option populators to take in a request option as input to avoid having to new one up per request.
  • Added proper continuation token support for StartFromBeginning, StartFromTime, and StartFromNow.
  • Switched over to CosmosElement for continuation token parsing and serialization
    ** This make the code exceptionless instead of throwing a newtonsoft exception
    ** Allows for continuation tokens to be composed
    ** Allows for continuation tokens to be serialized in binary
  • ChangeFeed does not modify HasMoreResults on 304s.
    ** This avoids having to serialize to a continuation token and rehydrating the iterator everytime this happens.
    ** Also allows the user to write a customer retry handler on 304s and also implement client side long poll if they want
  • Correctly aggregates the charge for 304 pages that we skipped for the user
  • Buffers exceptions and returns an empty page with the aggregated stats
  • Added ChangeFeed logic to InMemoryContainer so that we can unit test without the emulator.

The grammar for the cross partition change feed token is:

<composite_change_feed_token> : '[' <change_feed_token>, ... <change_feed_token> ']'
<change_feed_token> : '{' "FeedRange" ':' <feed_range>, "State" ':' <state> '}'
<feed_range> : '{' "type" ':' <feed_range_type>, "value" ':' <feed_range_value> '}'
<feed_range_type> : "Logical Partition Key" | "Physical Partition Key Range Id" | "Effective Partition Key Range"
<feed_range_value> : <logical_partition_key_json_string> | <physical_partition_key_id> | <epk_range>
<epk_range> : '{' "min": <string>, "max": <string>'}'
<state> : <start_from_begining_state> | <start_from_now_state> | <start_from_time_state> | <start_from_continuation_state>

An example token is:

[
    {
        "FeedRange":{
            "type":"Effective Partition Key Range",
            "value":{
                "min":"BF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF",
                "max":"DF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF"
            }
        },
        "State":{
            "type":"beginning"
        }
    },
    {
        "FeedRange":{
            "type":"Effective Partition Key Range",
            "value":{
                "min":"DF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF",
                "max":""
            }
        },
        "State":{
            "type":"continuation",
            "value":"88"
        }
    },
    {
        "FeedRange":{
            "type":"Effective Partition Key Range",
            "value":{
                "min":"7F-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF",
                "max":"9F-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF"
            }
        },
        "State":{
            "type":"now"
        }
    },
    {
        "FeedRange":{
            "type":"Effective Partition Key Range",
            "value":{
                "min":"3F-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF",
                "max":"5F-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF-FF"
            }
        },
        "State":{
            "type":"time",
            "value":"2020-10-14T08:00:45.0937045Z"
        }
    }
]

Fixes #1918

Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

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

Blocking on three things:

  1. The Emulator ContractTest change (see comment)
  2. The new behavior is throwing exceptions on 304, we don't throw exceptions on Not Modified currently. Why do are we doing it now? The previous behavior would simply return an empty page and HasMoreResults would be false, so the loop ended. What is the reasoning to add exceptions now?
  3. ChangeFeedOptions.EmitOldContinuationToken: What is the use case and what happens if the user leaves it enabled continuously?

@bchong95
Copy link
Contributor Author

Blocking on three things:

  1. The Emulator ContractTest change (see comment)
    responded to in comments.
  2. The new behavior is throwing exceptions on 304, we don't throw exceptions on Not Modified currently. Why do are we doing it now? The previous behavior would simply return an empty page and HasMoreResults would be false, so the loop ended. What is the reasoning to add exceptions now?
    responded to in comments.
  3. ChangeFeedOptions.EmitOldContinuationToken: What is the use case and what happens if the user leaves it enabled continuously?
    please read doc comments in source code.

ealsur
ealsur previously approved these changes Oct 19, 2020
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

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

Synced offline. The migration test and format change to support previous customers was cleared up.

@sboshra sboshra merged commit 312bb6e into master Oct 20, 2020
@sboshra sboshra deleted the users/brchon/ChangeFeed/UsePaginationLib branch October 20, 2020 17:24
ealsur added a commit that referenced this pull request Dec 3, 2020
…ntroduced in PR #1933 (#2042)

* Fixing the regression

* adding tests

Co-authored-by: Fabian Meiswinkel <[email protected]>
ealsur added a commit that referenced this pull request Dec 17, 2020
…ntroduced in PR #1933 (#2042)

* Fixing the regression

* adding tests

Co-authored-by: Fabian Meiswinkel <[email protected]>
@ghost
Copy link

ghost commented Dec 15, 2021

Closing due to in-activity, pease feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Feed StartFrom.Now() not respected with multiple physical partitions
4 participants