Azure cosmos diagnostics memory leak fix #28343

kushagraThapar · 2022-04-15T19:54:42Z

Problems ->

Current version of CosmosDiagnostics contains reference to storeResponseStatistics which contains reference to StoreResponse which contains CosmosDiagnostics hence creating a circular dependency in CosmosDiagnostics
Current version of CosmosDiagnostics contains reference to StoreResponse - with full byte[] content, never freeing up those byte[] content even after the response has been received from the service.
This increases memory consumption in case of retries from SDK, since the request is maintained throughout retries, containing all the storeResponse with full byte[] of contents.
Current version of CosmosDiagnostics contains reference to GlobalEndpointManager which contains reference to some caches, schedulers, ExecutorService, and various other things, which makes it difficult for JVM to collect garbage diagnostics or responses.
Current version of CosmosException refers to CosmosDiagnostics, which refers to DiagnosticsClientContext which is actually a reference to RxDocumentClientImpl - which exposes all information include caches, Store Models, and what not.

Solution ->

Removed CosmosDiagnostics from StoreResponse as it is not required there.
For CosmosResponse types, moved CosmosDiagnostics to RxDocumentServiceResponse initializing during creation of response.
Created serializable versions of StoreResponse, StoreResult, and CosmosException, and performing a deep copy with only instance members required for diagnostics.
Removed GlobalEndpointManager reference from CosmosDiagnostics and just passed it around for capturing regionsContacted
Only exposed DiagnosticsClientConfig in CosmosDiagnostics - which is only for configs serialization.

…y through store response

azure-sdk · 2022-04-15T20:04:02Z

API change check for com.azure:azure-cosmos

API changes have been detected in com.azure:azure-cosmos. You can review API changes here

API changes

+         @Warning public static CosmosDiagnostics createCosmosDiagnostics(DiagnosticsClientContext diagnosticsClientContext) 
-         @Warning public static CosmosDiagnostics createCosmosDiagnostics(DiagnosticsClientContext diagnosticsClientContext, GlobalEndpointManager globalEndpointManager) 
-         @Warning public static void recordGatewayResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest rxDocumentServiceRequest, StoreResponse storeResponse, CosmosException exception) 
-         @Warning public static void recordResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest request, StoreResult storeResult) 
+         @Warning public static void recordGatewayResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest rxDocumentServiceRequest, StoreResponse storeResponse, CosmosException exception, GlobalEndpointManager globalEndpointManager) 
+         @Warning public static void recordResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest request, StoreResult storeResult, GlobalEndpointManager globalEndpointManager)

kushagraThapar · 2022-04-15T22:30:34Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-15T22:30:48Z

Azure Pipelines successfully started running 1 pipeline(s).

.../azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientSideRequestStatistics.java

...cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/AddressSelector.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java

...e-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResponse.java

kushagraThapar · 2022-04-18T16:21:36Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-18T16:21:51Z

Azure Pipelines successfully started running 1 pipeline(s).

kushagraThapar · 2022-04-19T01:39:40Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-19T01:39:54Z

Azure Pipelines successfully started running 1 pipeline(s).

...ure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResult.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java

Netyyyy · 2022-04-20T07:24:13Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-20T07:24:18Z

Pull request contains merge conflicts.

FabianMeiswinkel

LGTM

FabianMeiswinkel · 2022-04-20T15:51:09Z

...osmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/IAddressResolver.java

@@ -15,4 +16,6 @@ Mono<AddressInformation[]> resolveAsync(
            boolean forceRefreshPartitionAddresses);

    int updateAddresses(URI serverKey);
+
+    GlobalEndpointManager getGlobalEndpointManager();


We can discuss when you are in the office - I don't quite get why we would need to expose the GlobalEndpointManager here? There should only be one instance of it - why keep the reference in the AddressResolver?

Agreed, worth discussing.

xinlian12 · 2022-04-20T17:45:46Z

...azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/SharedGatewayHttpClient.java

@@ -33,7 +32,7 @@ public static HttpClient getOrCreateInstance(HttpClientConfig httpClientConfig,
            }

            counter.incrementAndGet();
-            diagnosticsClientConfig.withGatewayHttpClientConfig(sharedGatewayHttpClient.effectiveHttpClientConfig);
+            diagnosticsClientConfig.withGatewayHttpClientConfig(httpClientConfig.toDiagnosticsString());


should we use sharedGatewayHttpClient.effectiveHttpClientConfig.toDiagnosticsString()

Any possibility that we are using a different httpClientConfig through out?

I removed the effectiveHttpClientConfig from SharedGatewayHttpClient class as it was referencing the same thing. Worth taking another look.

xinlian12 · 2022-04-20T18:03:14Z

.../azure-cosmos/src/test/java/com/azure/cosmos/implementation/CilentConfigDiagnosticsTest.java

        jsonGenerator.flush();
        ObjectNode objectNode = (ObjectNode) objectMapper.readTree(jsonWriter.toString());

        assertThat(objectNode.get("id").asInt()).isEqualTo(1);
        assertThat(objectNode.get("machineId").asText()).isEqualTo(machineId);
        assertThat(objectNode.get("numberOfClients").asInt()).isEqualTo(2);
-        assertThat(objectNode.get("consistencyCfg").asText()).isEqualTo("(consistency: null, mm: false, prgns: [])");
+        assertThat(objectNode.get("consistencyCfg").asText()).isEqualTo("(consistency: null, mm: false, prgns: [null])");


why the change here?

Will discuss offline, had to change the default preferredRegions to null string if empty.

xinlian12

LGTM, thanks

Netyyyy · 2022-04-21T03:03:58Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-21T03:04:12Z

Azure Pipelines successfully started running 1 pipeline(s).

…essResolver

aayush3011 · 2022-04-22T17:56:46Z

/azp run java - cosmos - tests

azure-pipelines · 2022-04-22T17:57:00Z

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel

Changelog entries for azure-cosmos and both spark versions are missing - otherwise LGTM

kushagraThapar added 2 commits April 15, 2022 12:47

Improved diagnostics and removed circular dependency

8144792

Fixed memory leak on CosmosDiagnostics by removing circular dependenc…

1385e4e

…y through store response

kushagraThapar requested review from FabianMeiswinkel, kirankumarkolli, mbhaskar, simplynaveen20, xinlian12, milismsft and aayush3011 as code owners April 15, 2022 19:54

ghost added the Cosmos label Apr 15, 2022

Fixed addressResolutionStatistics test error message

da85b69

xinlian12 reviewed Apr 15, 2022

View reviewed changes

.../azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientSideRequestStatistics.java Show resolved Hide resolved

xinlian12 reviewed Apr 15, 2022

View reviewed changes

...cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/AddressSelector.java Outdated Show resolved Hide resolved

FabianMeiswinkel reviewed Apr 17, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java Show resolved Hide resolved

FabianMeiswinkel reviewed Apr 17, 2022

View reviewed changes

...e-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResponse.java Show resolved Hide resolved

Fixed Diagnostics for CosmosException

61d2e5e

kushagraThapar added 3 commits April 18, 2022 18:21

Fixed memory leak related to DiagnosticsClientContext

f34d82a

Resolved conflicts

f3e242e

Fixed merge issues

d953c64

milismsft reviewed Apr 19, 2022

View reviewed changes

...ure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResult.java Outdated Show resolved Hide resolved

milismsft reviewed Apr 19, 2022

View reviewed changes

...ure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResult.java Outdated Show resolved Hide resolved

milismsft reviewed Apr 19, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java Show resolved Hide resolved

Fixed Connection state listener test

af349af

kushagraThapar added 2 commits April 19, 2022 00:31

Fixed URI and SessionToken Strings

b2266d9

Code review comments

f07b297

milismsft approved these changes Apr 20, 2022

View reviewed changes

Merged latest main and resolved conflicts

5f4bfd5

FabianMeiswinkel approved these changes Apr 20, 2022

View reviewed changes

xinlian12 reviewed Apr 20, 2022

View reviewed changes

xinlian12 approved these changes Apr 20, 2022

View reviewed changes

Netyyyy mentioned this pull request Apr 21, 2022

Upgrade external dependencies' version to spring-boot 2.6.6 #28280

Merged

kushagraThapar added 2 commits April 22, 2022 07:18

Merge branch 'main' into azure-cosmos-diagnostics-memory-leak-fix

3aa9b0b

Added GlobalEndpointManager to TransportClient and removed from IAddr…

603e6f8

…essResolver

FabianMeiswinkel approved these changes Apr 22, 2022

View reviewed changes

kushagraThapar added 2 commits April 22, 2022 14:36

Fixed returning instance of GlobalEndpointManager in tests

dba29c4

Added changelog for this memory leak fix

bac26d6

kushagraThapar merged commit 9208771 into Azure:main Apr 22, 2022

kushagraThapar mentioned this pull request Sep 23, 2022

[BUG]: java.lang errors causes CosmosDB query thread hang due to Reactor library issue #28707

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure cosmos diagnostics memory leak fix #28343

Azure cosmos diagnostics memory leak fix #28343

kushagraThapar commented Apr 15, 2022 •

edited

azure-sdk commented Apr 15, 2022

kushagraThapar commented Apr 15, 2022

azure-pipelines bot commented Apr 15, 2022

kushagraThapar commented Apr 18, 2022

azure-pipelines bot commented Apr 18, 2022

kushagraThapar commented Apr 19, 2022

azure-pipelines bot commented Apr 19, 2022

Netyyyy commented Apr 20, 2022

azure-pipelines bot commented Apr 20, 2022

FabianMeiswinkel left a comment

FabianMeiswinkel Apr 20, 2022

kushagraThapar Apr 21, 2022

xinlian12 Apr 20, 2022 •

edited

kushagraThapar Apr 21, 2022

xinlian12 Apr 20, 2022

kushagraThapar Apr 21, 2022

xinlian12 left a comment

Netyyyy commented Apr 21, 2022

azure-pipelines bot commented Apr 21, 2022

aayush3011 commented Apr 22, 2022

azure-pipelines bot commented Apr 22, 2022

FabianMeiswinkel left a comment

Azure cosmos diagnostics memory leak fix #28343

Azure cosmos diagnostics memory leak fix #28343

Conversation

kushagraThapar commented Apr 15, 2022 • edited

azure-sdk commented Apr 15, 2022

kushagraThapar commented Apr 15, 2022

azure-pipelines bot commented Apr 15, 2022

kushagraThapar commented Apr 18, 2022

azure-pipelines bot commented Apr 18, 2022

kushagraThapar commented Apr 19, 2022

azure-pipelines bot commented Apr 19, 2022

Netyyyy commented Apr 20, 2022

azure-pipelines bot commented Apr 20, 2022

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

FabianMeiswinkel Apr 20, 2022

Choose a reason for hiding this comment

kushagraThapar Apr 21, 2022

Choose a reason for hiding this comment

xinlian12 Apr 20, 2022 • edited

Choose a reason for hiding this comment

kushagraThapar Apr 21, 2022

Choose a reason for hiding this comment

xinlian12 Apr 20, 2022

Choose a reason for hiding this comment

kushagraThapar Apr 21, 2022

Choose a reason for hiding this comment

xinlian12 left a comment

Choose a reason for hiding this comment

Netyyyy commented Apr 21, 2022

azure-pipelines bot commented Apr 21, 2022

aayush3011 commented Apr 22, 2022

azure-pipelines bot commented Apr 22, 2022

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

kushagraThapar commented Apr 15, 2022 •

edited

xinlian12 Apr 20, 2022 •

edited