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

ChangeFeed : Adds ChangeFeedStartFrom to support StartTimes x FeedRanges #1725

Merged
merged 33 commits into from
Aug 11, 2020

Conversation

bchong95
Copy link
Contributor

@bchong95 bchong95 commented Jul 23, 2020

Internal ChangeFeed : Adds StartFrom x FeedRange

Description

This PR removes FeedRange from ChangeFeedRequestOptions and instead moves it as a cross product to StartFrom. This is to avoid a user setting a continuation token (that already has the FeedRange) with another FeedRange. The full details for that can be found here: #1686.

This implies that we need to remove the PartitionKey + ChangeFeedRequestOptions overloads, since users might set PartitionKey and the FeedRange in ChangeFeedRequestOptions, which is ambigious for which should win. This implies that FeedRange needs a static constructor for users to create a FeedRange that points to a single logical partition:

public static FeedRange CreateFromPartitionKey(PartitionKey partitionKey) => new FeedRangePartitionKey(partitionKey);

With that in mind the current API for StartFrom looks like this:

public abstract class ChangeFeedStartFrom
{
	public static ChangeFeedStartFrom Now() => Now(FeedRangeEpk.FullRange);

	public static ChangeFeedStartFrom Now(FeedRange feedRange);

	public static ChangeFeedStartFrom Time(DateTime dateTime) => Time(dateTime, FeedRangeEpk.FullRange);

	public static ChangeFeedStartFrom Time(DateTime dateTime, FeedRange feedRange);

	public static ChangeFeedStartFrom ContinuationToken(string continuation) => new ChangeFeedStartFromContinuation(continuation);

	public static ChangeFeedStartFrom Beginning() => Beginning(FeedRangeEpk.FullRange);

	public static ChangeFeedStartFrom Beginning(FeedRange feedRange);
}

Which is a slight variation from what is proposed in the github issue. It opts for additional methods instead of overloads or optional parameters. This is to make the intellisense on the public API more clear and also allows for non compile time default values (like EpkRange.FullRange).

The ChangeFeed API now looks like this:

public override FeedIterator GetChangeFeedStreamIterator(
	ChangeFeedStartFrom changeFeedStartFrom,
	ChangeFeedRequestOptions changeFeedRequestOptions = null)

public override FeedIterator<T> GetChangeFeedIterator<T>(
	ChangeFeedStartFrom changeFeedStartFrom,
	ChangeFeedRequestOptions changeFeedRequestOptions = null)

With ChangeFeedStartFrom as a required field. This eliminates ambiguous defaults.

A new derived class from start from was also added (StartFromContinuationAndFeedRange), which is not exposed to the user. Essentially a user will pass in StartFromContinuation and we will deserialize and convert it to a StartFromContinuationAndFeedRange.

sboshra
sboshra previously approved these changes Jul 23, 2020
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:

Base automatically changed from revert-1684-users/jawilley/contract/changefeed_revert to master July 24, 2020 01:03
@sboshra sboshra dismissed their stale review July 24, 2020 01:03

The base branch was changed.

@bchong95 bchong95 self-assigned this Jul 30, 2020
Microsoft.Azure.Cosmos/src/ChangeFeedStartFrom.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/ChangeFeedStartFrom.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/ChangeFeedStartFrom.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/ChangeFeedStartFrom.cs Outdated Show resolved Hide resolved
Debug.Assert(activityScope != null &&
(operationType != OperationType.SqlQuery || operationType != OperationType.Query || operationType != OperationType.QueryPlan),
"There should be an activity id already set");
//Debug.Assert(activityScope != null &&
Copy link
Member

Choose a reason for hiding this comment

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

Which scenarios void this assert?

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually looks like a bug I introduced. I will send out a fix later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We run all tests in release mode, so adding a debug assert doesn't actually do anything. We should add code analysis to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fix is merged in.

@kirankumarkolli
Copy link
Member

    ///     FeedRange = feedRanges[0]

Refresh, also any other related documentation impact.


Refers to: Microsoft.Azure.Cosmos/src/Resource/Container/Container.cs:1233 in d098458. [](commit_id = d098458, deletion_comment = False)

@kirankumarkolli
Copy link
Member

    ///     FeedRange = feedRanges[0]

Refresh


Refers to: Microsoft.Azure.Cosmos/src/Resource/Container/Container.cs:1191 in d098458. [](commit_id = d098458, deletion_comment = False)

ealsur
ealsur previously approved these changes Aug 10, 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.

Please update the public examples on the Container.cs file with the new API

@bchong95 bchong95 dismissed stale reviews from ealsur and j82w via 33a803e August 10, 2020 16:16
@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.

5 participants