-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Batch kill in azure #15770
Batch kill in azure #15770
Conversation
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java
Outdated
Show resolved
Hide resolved
...re/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentKiller.java
Outdated
Show resolved
Hide resolved
...re/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentKiller.java
Outdated
Show resolved
Hide resolved
...re/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentKiller.java
Outdated
Show resolved
Hide resolved
…d/storage/azure/AzureDataSegmentKiller.java Co-authored-by: Suneet Saldanha <suneet@apache.org>
...ons-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureStorageTest.java
Fixed
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.
Mostly looks good. 1 clarifying question
...ons-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureStorageTest.java
Fixed
Show resolved
Hide resolved
); | ||
} | ||
|
||
@Test(expected = RuntimeException.class) |
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 tried tracing the code to understand when azureStorage#batchDeleteFiles can throw a runtime exception, but couldn't figure it out. Can you elaborate on when that is possible and why we would want the data segment killer to re-throw the runtime exception instead of wrapping it in a SegmentLoadingException. The interface says a SegmentLoadingException should be thrown if there is an issue with deleting the segments.
@Test(expected = RuntimeException.class) | |
@Test(expected = SegmentLoadingException.class) |
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 tried tracing the code to understand when azureStorage#batchDeleteFiles can throw a runtime exception, but couldn't figure it out. Can you elaborate on when that is possible and why we would want the data segment killer to re-throw the runtime exception instead of wrapping it in a SegmentLoadingException. The interface says a SegmentLoadingException should be thrown if there is an issue with deleting the segments.
it could potentially throw a exception when trying to construct the client (before making the requests to azure), those exceptions would not get caught and should propagate up. it's probably not that common of a scenario though
Description
Implement batch kill in azure to speed up segment killing.
I ran a quick test with 50k segments and kill sped up from 35 mins to ~2 minutes.
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
I tried to keep this implementation analagous to the S3DataSegmentKiller. I used the azure batch api to do this. One thing I had to do was ignore the descriptor.json files but I think this is okay because those were discontinued in 2019 and the azure extension was made core in 2020. If someone happens to have segments with descriptor.json from that period the descriptor.json will be left in deep storage and have to get killed manually.
I think this is worth it because otherwise I would have to fire off 2x as many batches to azure to delete all the descriptor.json files which generally don't exist. These failed requests also mess up the logs.
Release note
Speed up killing of segments in azure
Key changed/added classes in this PR
AzureDataSegmentKiller
AzureStorage
TheirBaz
This PR has: