-
Notifications
You must be signed in to change notification settings - Fork 2k
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
findById Optimization #35261
findById Optimization #35261
Conversation
...ure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/core/CosmosTemplateIT.java
Show resolved
Hide resolved
@TheovanKraay will this change be ported to the spring boot 3 branch? |
...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java
Outdated
Show resolved
Hide resolved
...ng/azure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/domain/BasicItem.java
Show resolved
Hide resolved
...ng/azure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/domain/BasicItem.java
Show resolved
Hide resolved
Oh I had not intended to as I expected this to be merged earlier as it’s only a small change. Can you pull from main when merged? The changes are very small. Or will Spring Boot 3 branch be merged very soon? |
API change check API changes are not detected in this pull request. |
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -351,6 +351,15 @@ public <T> T findById(Object id, Class<T> domainType, PartitionKey partitionKey) | |||
public <T> T findById(String containerName, Object id, Class<T> domainType) { | |||
Assert.hasText(containerName, "containerName should not be null, empty or only whitespaces"); | |||
Assert.notNull(domainType, "domainType should not be null"); | |||
CosmosContainerProperties containerProperties = getContainerProperties(containerName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this add another round trip to the server for every findById
? Can we cache this somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh - yes, of course - fixed :-)
...ring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/ReactiveCosmosTemplate.java
Show resolved
Hide resolved
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -5,6 +5,7 @@ | |||
#### Features Added | |||
|
|||
#### Breaking Changes | |||
* Optimization for `findById(ID id)` so that it will execute point reads where id is also the partition key, and log a warning where it is not. Technically a breaking change, but new behaviour is more optimal, especially for large containers with many partitions - see [PR 35261](https://github.com/Azure/azure-sdk-for-java/pull/35261). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this still technically a breaking change? I don't think it is anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be breaking if behaviour changes. I’m not sure on what we define as “breaking”. Happy to change this.
return findById(id, domainType, new PartitionKey(id)); | ||
} | ||
if (!this.pointReadWarningLogged) { | ||
LOGGER.warn("The partitionKey is not id!! Consider using findById(ID id, PartitionKey partitionKey) instead. See https://aka.ms/PointReadsInSpring for more info."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the error message here to inlcude at least 1 sentence for the motivation like.
The partitionKey is not id!! Consider using findById(ID id, PartitionKey partitionKey) instead to avoid the need for using a cross partition query which results in higher latency and cost than necessary. See https://aka.ms/PointReadsInSpring for more info.
...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for few minor comments about log message and changelog text
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/check-enforcer override |
/azp run java - spring - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/check-enforcer override |
BASIC_ITEM.getId(), BasicItem.class); | ||
assertEquals(result, BASIC_ITEM); | ||
assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics()).isNotNull(); | ||
assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add this line to make sure it is a point read ->
assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics().toString().contains("\"requestOperationType\":\"Read\"")).isTrue();
.consumeNextWith(actual -> Assert.assertEquals(actual, BASIC_ITEM)) | ||
.verifyComplete(); | ||
assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics()).isNotNull(); | ||
assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics().toString().contains("\"requestOperationType\":\"Read\"")).isTrue();
public void testFindById() { | ||
// test findById (ID id) cross partition | ||
final Address result = repository.findById(TEST_ADDRESS1_PARTITION1.getPostalCode()).get(); | ||
assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNotNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should check for the operation type is query. Similar to above.
/azp run java - spring - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work @TheovanKraay , thank you!!
* findById Optimization * reformat * add hashcode override and licence to BasicItem * improve point read test * address linting errors * fix linting errors * fix linting * some enhancements * fix build errors * fix testFindByIdNull integration test failure * add return null for CosmosAccessException * address Fabian review comment * update change log * address review comments * fix build errors * fix build errors * fix build errors * fix build errors * add testFindByIdWithInvalidId * add missing license header * add repository api checks for point read/query * change pointReadWarningLogged to primitive * edit change log and warning text * add extra checks per Kushagra's comments * Update CHANGELOG.md --------- Co-authored-by: Theo van Kraay <thvankra@microsoft.com> Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
This reverts commit a7b417c.
* findById Optimization * reformat * add hashcode override and licence to BasicItem * improve point read test * address linting errors * fix linting errors * fix linting * some enhancements * fix build errors * fix testFindByIdNull integration test failure * add return null for CosmosAccessException * address Fabian review comment * update change log * address review comments * fix build errors * fix build errors * fix build errors * fix build errors * add testFindByIdWithInvalidId * add missing license header * add repository api checks for point read/query * change pointReadWarningLogged to primitive * edit change log and warning text * add extra checks per Kushagra's comments * Update CHANGELOG.md --------- Co-authored-by: Theo van Kraay <thvankra@microsoft.com> Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
* findById Optimization (#35261) * findById Optimization * reformat * add hashcode override and licence to BasicItem * improve point read test * address linting errors * fix linting errors * fix linting * some enhancements * fix build errors * fix testFindByIdNull integration test failure * add return null for CosmosAccessException * address Fabian review comment * update change log * address review comments * fix build errors * fix build errors * fix build errors * fix build errors * add testFindByIdWithInvalidId * add missing license header * add repository api checks for point read/query * change pointReadWarningLogged to primitive * edit change log and warning text * add extra checks per Kushagra's comments * Update CHANGELOG.md --------- Co-authored-by: Theo van Kraay <thvankra@microsoft.com> Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com> * Update sdk/spring/azure-spring-data-cosmos/CHANGELOG.md Co-authored-by: Kushagra Thapar <kuthapar@microsoft.com> --------- Co-authored-by: Theo van Kraay <theo.van@microsoft.com> Co-authored-by: Theo van Kraay <thvankra@microsoft.com> Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com> Co-authored-by: Fabian Meiswinkel <fabianm@microsoft.com> Co-authored-by: Kushagra Thapar <kuthapar@microsoft.com>
Description
The current default behaviour of
findById(ID id)
is that it will always execute a cross partition key query. This is not an optimal behaviour for containers with big datasets/large numbers of partitions. This change is an optimization to makefindById(ID id)
execute a point read when partitionKey isid
, and log a warning when it is a field other than id. The new default behaviour is almost certain to be what user intends. Current behaviour can potentially cause significant unintended latency issues.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines