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.

Sign up for GitHub

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

Jump to bottom

ChangeFeedRequestOptions Refactor #1332

Merged
merged 56 commits into from
Jun 22, 2020
Merged

ChangeFeedRequestOptions Refactor #1332

merged 56 commits into from
Jun 22, 2020

Conversation

bchong95
Copy link
Contributor

@bchong95 bchong95 commented Apr 1, 2020

ChangeFeedRequestOptions Refactor

Description

Refactored ChangeFeedRequestOptions to have the following design points:

  • Avoid users setting mutually exclusive fields like continuation token and start from
  • StartFrom Beginning is just not setting a start from and avoid setting to a magic number like DateTime.MinValue
  • Enforces that StartTime is always in correct UTC format.
  • Adds visitor so that work won't break when new types are added
  • Made static constructors for intellisense discoverablity.
  • Removed static methods that enrich the request message, when it could be done in PopulateRequestMessage.

@bchong95 bchong95 self-assigned this Apr 1, 2020
@bchong95 bchong95 requested review from ealsur and j82w April 1, 2020 23:32
ealsur
ealsur requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@ealsur ealsur left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why are you Changing the default from Start From Now to Start From Beginning? This is a breaking change.

While I agree that this solves the problem of people passing invalid combination of values on the headers it does make the most common cases harder and forces users to write more code to achieve the same. Isn't there a middle ground?

By putting the continuation in the ChangeFeedRequestOptions the public surface becomes more complex than it should be if I just want to pass the continuation and it's not inline with any of our other iterators (GetItemQueryIterator, etc).

It is also breaking Change Feed Processor start from beginning on Multi Master accounts (see comment on ResultSetIteratorUtils.cs.

Microsoft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedIteratorCore.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/RequestOptions/ChangeFeedRequestOptions.cs Outdated Show resolved Hide resolved
...soft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedPartitionKeyResultSetIteratorCore.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Resource/QueryResponses/StandByFeedIteratorCore.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemChangeFeedTests.cs Outdated Show resolved Hide resolved
...e.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/FeedToken/ChangeFeedIteratorCoreTests.cs Outdated Show resolved Hide resolved
...e.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/FeedToken/ChangeFeedIteratorCoreTests.cs Outdated Show resolved Hide resolved
...e.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/FeedToken/ChangeFeedIteratorCoreTests.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/StandByFeedContinuationTokenTests.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/StandByFeedContinuationTokenTests.cs Outdated Show resolved Hide resolved
sboshra
sboshra previously approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

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

:shipit:

ealsur
ealsur previously approved these changes Apr 20, 2020
View reviewed changes
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.

Awesome work on the new types!

@sboshra sboshra merged commit a2726ba into master Jun 22, 2020
@sboshra sboshra deleted the users/brchon/ChangeFeed/RequestOptionsRefactor branch June 22, 2020 23:50
@jorgensigvardsson
Copy link

Is this work towards making the ChangeFeedIterator part of the non-preview releases? I have issues with the feed processor, and I need to use the change feed iterators to solve my problem. Are there any planned dates for when the ChangeFeedIterator becomes generally available?

ealsur
ealsur reviewed Jun 26, 2020
View reviewed changes
Microsoft.Azure.Cosmos/src/FeedRange/Continuations/FeedRangeCompositeContinuation.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/FeedRange/FeedRanges/FeedRangeRequestMessagePopulatorVisitor.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/RequestOptions/ChangeFeedRequestOptions.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedRangeIteratorCore.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedIteratorCore.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedIteratorCore.cs Show resolved Hide resolved
...Azure.Cosmos/src/FeedRange/FeedRanges/FeedRangeContinuationRequestMessagePopulatorVisitor.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/FeedRange/FeedRanges/FeedRangePartitionKeyRangeExtractor.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/Container.cs Show resolved Hide resolved
j82w pushed a commit that referenced this pull request Jul 3, 2020
Revert "ChangeFeedRequestOptions Refactor (#1332)"
63014b1 
This reverts commit a2726ba.
j82w added a commit that referenced this pull request Jul 6, 2020
@j82w
Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)
b873a89 
* Revert "ChangeFeedRequestOptions Refactor (#1332)"

This reverts commit a2726ba.

* Update preview contract
@j82w j82w mentioned this pull request Jul 6, 2020
Closed
bchong95 added a commit that referenced this pull request Jul 8, 2020
@bchong95
Revert "Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)"
0d924ee 
This reverts commit b873a89.
kirankumarkolli pushed a commit that referenced this pull request Jul 11, 2020
@bchong95 @kirankumarkolli
ChangeFeedRequestOptions Refactor (#1332)
2a8020f 
* made it so that it is clear that ifmatchetag and ifnonematchetag are not used

* added start from types

* added visitor

* added visitors

* wired throughout the codebase

* set explicit default

* fixed tests

* fixed tests

* resolved iteration comments

* merged

* fixed tests

* updated preview API

* more preview build errors

* more build issues

* grr

* fixed tests

* fixed build

* added feed range to feed options to remove duplicated state

* preview build

* fixed NRE

* preview build

* asdf

* fixed preview build errors

* need to investigate why there are test failures

* fixed request options tests

* asdf

* need to investigate in a clean brach

* need to figure out what is wrong with the continuation token

* fixed some tests

* fixed XML comments

* fixed infinite loop

* need to investigate some changes

* fixed one test

* fixed continuation token bug

* added clone method

* fixed continuation token bug in standbyfeediterator

* fixed another continuaiton token bug

* need to remove because of start time

* fixed test that needed to start from the begining

* need to investigate this on master

* updated continuation token

* fixed diagnostics scope
kirankumarkolli pushed a commit that referenced this pull request Jul 11, 2020
@j82w @kirankumarkolli
Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)
ccf56f4 
* Revert "ChangeFeedRequestOptions Refactor (#1332)"

This reverts commit a2726ba.

* Update preview contract
@bchong95 bchong95 mentioned this pull request Jul 22, 2020
Merged
@ealsur ealsur mentioned this pull request Jul 24, 2020
Merged
sboshra added a commit that referenced this pull request Jul 24, 2020
@bchong95 @sboshra
Revert : Add "Revert "ChangeFeedRequestOptions Refactor (#1332)"" (#1692
ec0ee50 
)

* Revert "Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)"

This reverts commit b873a89.

* marking test as ignored as agreed upon

Co-authored-by: Samer Boshra <sboshra@microsoft.com>
sboshra added a commit that referenced this pull request Jul 24, 2020
@bchong95 @sboshra
Internal ChangeFeed : Fixes Build Breaks (#1717)
9b655af 
* Revert "Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)"

This reverts commit b873a89.

* marking test as ignored as agreed upon

* added migration scenario

* increased the priority of feed range continuation visitor

* added a comment

* made request changes

* manual rename

* resolved iteration comments

Co-authored-by: Samer Boshra <sboshra@microsoft.com>
sboshra added a commit that referenced this pull request Aug 11, 2020
@bchong95 @sboshra @j82w
ChangeFeed : Adds ChangeFeedStartFrom to support StartTimes x FeedRan…
dccd915 
…ges (#1725)

* Revert "Revert "ChangeFeedRequestOptions Refactor (#1332)" (#1684)"

This reverts commit b873a89.

* marking test as ignored as agreed upon

* wired up changes

* resolved iteration comments

* merged

* made start from a required field

* added feedback from API review

* resolved iteration comments

* fixed tests

* fixed feed range docs

* removed unreachable code

* fixed tests

* fixed tests

* fixed mocks

* resolved iteration comments

* fixed build issue

* fixed test build errors

* more build fixes

* updated preview API json

Co-authored-by: Samer Boshra <sboshra@microsoft.com>
Co-authored-by: j82w <j82w@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabianMeiswinkel FabianMeiswinkel FabianMeiswinkel left review comments

@ealsur ealsur ealsur left review comments

@bartelink bartelink bartelink left review comments

@sboshra sboshra sboshra approved these changes

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli kirankumarkolli is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

@j82w j82w Awaiting requested review from j82w

@khdang khdang Awaiting requested review from khdang khdang is a code owner

Assignees

@bchong95 bchong95

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bchong95 @jorgensigvardsson @bartelink @ealsur @j82w @sboshra @FabianMeiswinkel