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] Change Feed: Fixes estimator diagnostics #1930

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Oct 13, 2020

Description

The Estimator was using a Partitioner to issue concurrent estimations and construct the result pages, but the diagnostics were being added concurrently through a thread-unsafe method.

This PR removes the Partitioner (which really was not adding any benefit) and makes sure the diagnostics are added non-concurrently.

This PR also removes flakyness in tests at https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/ChangeFeed/ChangeFeedEstimatorIteratorTests.cs due to randomly not having the expected diagnostics.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

@ealsur ealsur self-assigned this Oct 13, 2020
@ealsur ealsur added the bug Something isn't working label Oct 13, 2020
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

LGTM

@FabianMeiswinkel FabianMeiswinkel merged commit b65f597 into master Oct 13, 2020
@FabianMeiswinkel FabianMeiswinkel deleted the users/ealsur/estimatorpartitioner branch October 13, 2020 18:47
@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
bug Something isn't working ChangeFeed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants