(perf) server: batch SQL Metadata deleteSegments#14639
Merged
kfaraz merged 4 commits intoapache:masterfrom Jul 23, 2023
Merged
Conversation
jasonk000
force-pushed
the
batch-sql-deleteSegments
branch
from
3758d7a to
d57c891
Compare
10 tasks
Contributor
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
The overall approach looks good to me. I left a few comments around batch size and transaction code blocks. Thanks!
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java
Show resolved
Hide resolved
kfaraz
reviewed
Jul 22, 2023
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Show resolved
Hide resolved
kfaraz
reviewed
Jul 22, 2023
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
kfaraz
reviewed
Jul 22, 2023
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
kfaraz
requested changes
Jul 22, 2023
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Thanks for taking this up, @jasonk000 ! I have left some suggestions.
…adata pr feedback: - extract batch update and delete data generation outside of the SQL transaction, - avoid a query altogether if there are no segments to add, - improvement to logging
removed the updateSegmentMetadata batching since it is dead code
kfaraz
reviewed
Jul 23, 2023
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
Show resolved
Hide resolved
pr feedback - improve logging accuracy - restore missing newline
Contributor
Author
jasonk000
changed the title
kfaraz
approved these changes
Jul 23, 2023
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Thanks for the quick fix, @jasonk000 !
3 tasks
Contributor
|
A bit late to the PR. Change looks good to me. Thank you for the PR! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Related to #14634.
Description
Introduce batching to mitigate some scaling challenges while managing lots of segments.
This PR introduces changes to
IndexerSQLMetadataStorageCoordinatorto use the JDBI PreparedBatch instead of issuing single update statements inside a transaction.Context - in our environment, bulk cleanup of old segments (O(thousands)) stalls the overlord, because the overlord is issuing delete statements. These delete statements are done while holding the TaskLockbox lock, which is done from the TaskQueue, so the whole overlord locks up until the delete statements are complete. By pushing these into a single bulk transaction we see a meaningful improvement: previously ~1000 rows/sec removed, after ~1800 rows/sec removed.
Key changed/added classes in this PR
IndexerSQLMetadataStorageCoordinator: use PreparedBatch instead of single statements.This PR has: