findById Optimization by TheovanKraay · Pull Request #35261 · Azure/azure-sdk-for-java

TheovanKraay · 2023-06-01T21:17:51Z

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 make findById(ID id) execute a point read when partitionKey is id, 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:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

saragluna · 2023-06-02T03:00:45Z

@TheovanKraay will this change be ported to the spring boot 3 branch?

TheovanKraay · 2023-06-02T06:47:46Z

@TheovanKraay will this change be ported to the spring boot 3 branch?

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?

azure-sdk · 2023-06-02T12:01:07Z

API change check

API changes are not detected in this pull request.

TheovanKraay · 2023-06-02T15:53:46Z

/azp run java - cosmos - tests

azure-pipelines · 2023-06-02T15:53:58Z

Azure Pipelines successfully started running 1 pipeline(s).

Pilchie · 2023-06-02T16:35:23Z

    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);


Doesn't this add another round trip to the server for every findById? Can we cache this somewhere?

Ugh - yes, of course - fixed :-)

TheovanKraay · 2023-06-06T20:07:05Z

/azp run java - cosmos - tests

azure-pipelines · 2023-06-06T20:07:19Z

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel · 2023-06-07T07:16:32Z

 #### 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).


Why is this still technically a breaking change? I don't think it is anymore?

I thought it would be breaking if behaviour changes. I’m not sure on what we define as “breaking”. Happy to change this.

FabianMeiswinkel · 2023-06-07T07:18:23Z

+            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.");


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.

FabianMeiswinkel

LGTM except for few minor comments about log message and changelog text

TheovanKraay · 2023-06-07T08:36:28Z

/azp run java - cosmos - tests

azure-pipelines · 2023-06-07T08:36:40Z

Azure Pipelines successfully started running 1 pipeline(s).

TheovanKraay · 2023-06-07T14:46:13Z

/check-enforcer override

kushagraThapar · 2023-06-07T15:52:32Z

/azp run java - spring - tests

azure-pipelines · 2023-06-07T15:52:48Z

Azure Pipelines successfully started running 1 pipeline(s).

TheovanKraay · 2023-06-07T18:14:11Z

/check-enforcer override

kushagraThapar · 2023-06-08T20:31:36Z

+            BASIC_ITEM.getId(), BasicItem.class);
+        assertEquals(result, BASIC_ITEM);
+        assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics()).isNotNull();
+        assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNull();


We can also add this line to make sure it is a point read ->

assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics().toString().contains("\"requestOperationType\":\"Read\"")).isTrue();

kushagraThapar · 2023-06-08T20:31:57Z

+            .consumeNextWith(actual -> Assert.assertEquals(actual, BASIC_ITEM))
+            .verifyComplete();
+        assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics()).isNotNull();
+        assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNull();


Same here - assertThat(responseDiagnosticsTestUtils.getCosmosDiagnostics().toString().contains("\"requestOperationType\":\"Read\"")).isTrue();

kushagraThapar · 2023-06-08T20:32:21Z

+    public void testFindById() {
+        // test findById (ID id) cross partition
+        final Address result = repository.findById(TEST_ADDRESS1_PARTITION1.getPostalCode()).get();
+        assertThat(responseDiagnosticsTestUtils.getCosmosResponseStatistics()).isNotNull();


Here we should check for the operation type is query. Similar to above.

TheovanKraay · 2023-06-08T22:07:15Z

/azp run java - spring - tests

azure-pipelines · 2023-06-08T22:07:27Z

Azure Pipelines successfully started running 1 pipeline(s).

kushagraThapar

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>

findById Optimization

276b7ee

TheovanKraay requested review from Netyyyy, backwind1233, chenrujun, fangjian0423, hui1110, moarychan, saragluna, stliu and yiliuTo as code owners June 1, 2023 21:17

github-actions Bot added the azure-spring All azure-spring related issues label Jun 1, 2023

reformat

5f9adbe

trande4884 reviewed Jun 2, 2023

View reviewed changes

Comment thread ...ure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/core/CosmosTemplateIT.java

trande4884 reviewed Jun 2, 2023

View reviewed changes

Comment thread ...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java Outdated

trande4884 reviewed Jun 2, 2023

View reviewed changes

Comment thread ...ng/azure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/domain/BasicItem.java

trande4884 reviewed Jun 2, 2023

View reviewed changes

Comment thread ...ng/azure-spring-data-cosmos/src/test/java/com/azure/spring/data/cosmos/domain/BasicItem.java

add hashcode override and licence to BasicItem

48b1175

Theo van Kraay added 4 commits June 2, 2023 13:28

improve point read test

894d6f4

address linting errors

837f9cc

fix linting errors

fcdf27f

fix linting

716f0e1

trande4884 approved these changes Jun 2, 2023

View reviewed changes

Pilchie reviewed Jun 2, 2023

View reviewed changes

trande4884 reviewed Jun 2, 2023

View reviewed changes

Comment thread ...ring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/ReactiveCosmosTemplate.java

trande4884 approved these changes Jun 7, 2023

View reviewed changes

change pointReadWarningLogged to primitive

7e6424f

FabianMeiswinkel reviewed Jun 7, 2023

View reviewed changes

Comment thread ...azure-spring-data-cosmos/src/main/java/com/azure/spring/data/cosmos/core/CosmosTemplate.java

FabianMeiswinkel approved these changes Jun 7, 2023

View reviewed changes

edit change log and warning text

221e27f

TheovanKraay requested a review from kushagraThapar June 7, 2023 11:49

kushagraThapar reviewed Jun 8, 2023

View reviewed changes

add extra checks per Kushagra's comments

dfb0472

kushagraThapar approved these changes Jun 9, 2023

View reviewed changes

Update CHANGELOG.md

63b1e0c

kushagraThapar approved these changes Jun 9, 2023

View reviewed changes

TheovanKraay merged commit 8236c59 into Azure:main Jun 9, 2023

trande4884 added a commit that referenced this pull request Jun 9, 2023

Revert "findById Optimization (#35261)"

2b3ede3

This reverts commit a7b417c.

This was referenced Aug 2, 2023

multi-tenant sample enhancements Azure-Samples/azure-spring-boot-samples#714

Closed

multi-tenant sample enhancements Azure-Samples/azure-spring-boot-samples#716

Closed

Uh oh!

Conversation

TheovanKraay commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

All SDK Contribution checklist:

Uh oh!

Uh oh!

saragluna commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheovanKraay commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-sdk commented Jun 2, 2023

Uh oh!

TheovanKraay commented Jun 2, 2023

Uh oh!

azure-pipelines Bot commented Jun 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheovanKraay Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheovanKraay commented Jun 6, 2023

Uh oh!

azure-pipelines Bot commented Jun 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

Uh oh!

TheovanKraay commented Jun 7, 2023

Uh oh!

azure-pipelines Bot commented Jun 7, 2023

Uh oh!

TheovanKraay commented Jun 7, 2023

Uh oh!

kushagraThapar commented Jun 7, 2023

Uh oh!

azure-pipelines Bot commented Jun 7, 2023

Uh oh!

TheovanKraay commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheovanKraay commented Jun 8, 2023

Uh oh!

azure-pipelines Bot commented Jun 8, 2023

Uh oh!

kushagraThapar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

TheovanKraay commented Jun 1, 2023 •

edited

Loading

saragluna commented Jun 2, 2023 •

edited

Loading

TheovanKraay commented Jun 2, 2023 •

edited

Loading

TheovanKraay Jun 2, 2023 •

edited

Loading

kushagraThapar left a comment •

edited

Loading