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

PartitionRoutingHelper: Fix ReadFeed ArgumentNullException due to container cache miss #1512

Merged
merged 5 commits into from May 15, 2020
Merged

PartitionRoutingHelper: Fix ReadFeed ArgumentNullException due to container cache miss #1512

merged 5 commits into from May 15, 2020

Conversation

kirankumarkolli
Copy link
Member

@kirankumarkolli kirankumarkolli commented May 13, 2020 •
edited

ReadFeed with container cache misss will lead to NRE (from LINIQ).

Fix:

  1. Return expected payload so-that callers can retry (force-refresh)
  2. Avoid unnecessary array materializations (in simple paths, except reverse)
  3. Info log turned to warning for observability

@kirankumarkolli kirankumarkolli marked this pull request as ready for review May 14, 2020 14:46
@kirankumarkolli kirankumarkolli changed the title PartitionRoutingHelper: Fix missing cache miss PartitionRoutingHelper: Fix ReadFeed ArgumentNullException due to container cache miss May 14, 2020
bchong95
bchong95 previously approved these changes May 14, 2020
View reviewed changes
ealsur
ealsur previously approved these changes May 14, 2020
View reviewed changes
Microsoft.Azure.Cosmos/src/Routing/PartitionRoutingHelper.cs Outdated Show resolved Hide resolved
@kirankumarkolli kirankumarkolli dismissed stale reviews from ealsur and bchong95 via 11b5e34 May 14, 2020 18:50
@kirankumarkolli kirankumarkolli merged commit 46e8aca into master May 15, 2020
@kirankumarkolli kirankumarkolli deleted the users/kirankk/cache_miss_handling branch May 15, 2020 05:55
kirankumarkolli added a commit that referenced this pull request May 16, 2020
@kirankumarkolli
PartitionRoutingHelper: Fix ReadFeed ArgumentNullException due to con…
794b406 
…tainer cache miss (#1512)

* Test commit to treat null as missing info.

* UT asserting the resolvedRangeInfo with null values

* Marking missing routing map as warning

* reverting unnecessary change

* Using named parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bchong95 bchong95 bchong95 left review comments

@ealsur ealsur ealsur approved these changes

@j82w j82w j82w approved these changes

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

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kirankumarkolli @ealsur @bchong95 @j82w