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

Read many fix #35513

Merged
merged 11 commits into from
Jun 22, 2023
Merged

Read many fix #35513

merged 11 commits into from
Jun 22, 2023

Conversation

jeet1995
Copy link
Member

@jeet1995 jeet1995 commented Jun 18, 2023

Description

This PR fixes the issue when a point-read based 404 / 0 response drops existing point-read based documents from the resultant FeedResponse instance in spite of the ID of these documents being specified in the readMany request. The expected behavior is to ignore non-existent documents but fetch other documents.

Testing

Test: CosmosItemTest#readManyWithMultiplePartitionsAndSome404s

  • Create a multi-partition container - preferably with more than 2 physical partitions.
  • Ensure documents are created on each physical partition.
  • Query 1 document per physical partition using the FeedRange instance for the physical partition. This is done to ensure when readMany is used to query the documents back, it only uses point-reads to do so.
  • Choose n documents in which a non-existent ID is injected.
  • Read these documents back using the readMany API.
  • Verify if the count of fetched documents through readMany equals (total no. of physical partitions) - n
  • Repeat the above steps by varying n from 0 to the total no. of physical partitions.

Benchmarking results

  • The fix shows a marginal tail latency increase (in the order of 1 ms to 5 ms)
image

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@jeet1995
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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 - Thanks

@jeet1995
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeet1995
Copy link
Member Author

There are some metric-related tests which don't pass - would require some looking at. The CosmosConflictTests on the other passes locally.

@jeet1995
Copy link
Member Author

/check-enforcer override

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

Successfully merging this pull request may close these issues.

4 participants