Cosmos BulkExecutor: Fixing leak of threads with "cosmos-daemon-BulkExecutor-*" prefix #31082
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FabianMeiswinkel
requested review from
kushagraThapar,
kirankumarkolli,
xinlian12,
milismsft,
aayush3011 and
simorenoh
as code owners
ghost
added
the
|
API change check API changes are not detected in this pull request. |
…ulkWriter and BulkExecutor
FabianMeiswinkel
changed the title
FabianMeiswinkel
changed the title
sdk/cosmos/azure-cosmos-spark_3_2-12/src/main/scala/com/azure/cosmos/spark/BulkWriter.scala
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
xinlian12
reviewed
Sep 22, 2022
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncContainer.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Outdated
Show resolved
Hide resolved
kushagraThapar
approved these changes
Sep 22, 2022
There was a problem hiding this comment.
LGTM, thanks @FabianMeiswinkel
some minor comments, worth fixing the changelog.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncContainer.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Outdated
Show resolved
Hide resolved
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/batch/BulkExecutor.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
reviewed
Sep 22, 2022
...osmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/batch/BulkExecutorTest.java
Outdated
Show resolved
Hide resolved
xinlian12
approved these changes
Sep 22, 2022
fangjian0423
pushed a commit
to fangjian0423/azure-sdk-for-java
that referenced
this pull request
Sep 27, 2022
…xecutor-*" prefix (Azure#31082) * Cosmos BulkExecutor: Stopping the flush task to free up the daemon thread * Fixing thread leak due to race condition on bulk completion between BulkWriter and BulkExecutor * Iterating on fix * Adding unit test coverage * Updating changelog * Fixing changelog * Reacted to code review feedback * Update CosmosAsyncContainer.java * Update CosmosAsyncContainer.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A thread/memory leak was reported in this issue - #31077
When using a Spark structured streaming job to ingest data into Cosmos DB in micro batches there is a race condition that could cause BulkExecutor instances and their corresponding thread to not be cleaned-up properly. This can eventually reult in workers crashing with the following error: 'java.io.IOException: java.util.concurrent.ExecutionException: java.lang.OutOfMemoryError: unable to create new native thread'
The race condition described happens when the BulkWriter (in Spark connector) signals that all incoming operations have been consumed here -
azure-sdk-for-java/sdk/cosmos/azure-cosmos-spark_3_2-12/src/main/scala/com/azure/cosmos/spark/BulkWriter.scala
Line 524 in ea0b2e8
The BulkExecutor (in Java SDK) would react to this signal and cleanly close/clean-up everything - except if the BulKWriter is disposing the subsription here -
azure-sdk-for-java/sdk/cosmos/azure-cosmos-spark_3_2-12/src/main/scala/com/azure/cosmos/spark/BulkWriter.scala
Line 537 in ea0b2e8
To fix this instead we make sure that clean-up is happening independent of how the subscription is terminated (complete, cancel or error).
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines