You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "georgew5656 (via GitHub)" <gi...@apache.org> on 2024/03/06 19:13:37 UTC
[PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
georgew5656 opened a new pull request, #16065:
URL: https://github.com/apache/druid/pull/16065
Fixes a reporting bug with running k8s compaction tasks on azure.
### Description
There is logic in AbstractTask.cleanup that lets peons that are running standalone (currently only a thing with k8s mode) push report.json and a status.json after they have completed running. The report.json is a regular task report and the status.json is used by the KubernetesTaskRunner to check the status of the task.
This logic assumes that each task is run in isolation, e.g. that a peon will only run a single task. This assumption is not true in two cases, one case where a IndexTask is initialized and run inside a ParallelIndexSupervisorTask and one case where onr or many ParallelIndexSupervisorTask's are initialized and run inside a CompactionTask.
The problem with these cases is that both the interior and wrapper tasks try to run cleanup and push their report.json/status.json.
Currently this works okay if using amazon s3 as the backend, becasue the s3 client by default overwrites files and the wrapper task pushes its reports last, so the status of task will always be whatever the wrapper task says.
The AzureStorage client we wrote doesn't overwrite, so the wrapper task will actually throw a exception when trying to write its reports. The task ends up succeeding anyways because the KubernetesTaskRunner will see the k8s job exit and check status.json (which is successful since the interior task succeeded and pushed its status.json), but this is undesirable behavior.
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
The IndexTask already has a flag for telling it whether it is running standalone or inside another task and check this flag when deciding whether to cleanup. I pulled some of this logic into AbstractTask (since that is where the pushing reports logic is anyways), and added a interface on all tasks called shouldCleanup which defaults to true. The indexTask will return shouldCleanup=false if it is running inside another task. Similarily, ParallelIndexSupervisorTask will also return shouldCleanup=false if it is running inside a compaction task.
I also updated the azure client to overwrite files by default because this seems to be the default behavior in object stores and it's easier to keep things consistent.
#### Release note
Fixing bug with kubernetes ingestion and compaction
##### Key changed/added classes in this PR
* `AbstractTask`
* `IndexTask`
* `ParallelIndexSuperivsorTask`
* `AzureStorage`
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1516350446
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java:
##########
@@ -201,7 +201,11 @@ public void cleanUp(TaskToolbox toolbox, @Nullable TaskStatus taskStatus) throws
// clear any interrupted status to ensure subsequent cleanup proceeds without interruption.
Thread.interrupted();
Review Comment:
good point, i'm just gonna change the logic back to override cleanup, think its cleaner that way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "adithyachakilam (via GitHub)" <gi...@apache.org>.
adithyachakilam commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1516596054
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java:
##########
@@ -116,7 +116,7 @@ public void uploadBlockBlob(final File file, final String containerName, final S
try (FileInputStream stream = new FileInputStream(file)) {
// By default this creates a Block blob, no need to use a specific Block blob client.
// We also need to urlEncode the path to handle special characters.
- blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length());
+ blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length(), true);
Review Comment:
May be a comment would be good then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1516349185
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java:
##########
@@ -1108,13 +1108,12 @@ private static InputFormat getInputFormat(IndexIngestionSpec ingestionSchema)
}
@Override
- public void cleanUp(TaskToolbox toolbox, @Nullable TaskStatus taskStatus) throws Exception
+ protected boolean shouldCleanupTask()
Review Comment:
shouldCleanupTask is used in ParallelIndexSupervisorTask as well. i thought rather than copying the cleanupLogic in IndexTask (check the boolean and call super.cleanup() if true), it would be easier to add a new method that could be implemented, but i think what you say makes sense, i can change it back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 merged PR #16065:
URL: https://github.com/apache/druid/pull/16065
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "adithyachakilam (via GitHub)" <gi...@apache.org>.
adithyachakilam commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1515241732
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java:
##########
@@ -201,7 +201,11 @@ public void cleanUp(TaskToolbox toolbox, @Nullable TaskStatus taskStatus) throws
// clear any interrupted status to ensure subsequent cleanup proceeds without interruption.
Thread.interrupted();
Review Comment:
In previous code, if `isStandAloneTask=true`, even this line doesn't gets executed. Would it create any problems now ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1515608613
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java:
##########
@@ -1108,13 +1108,12 @@ private static InputFormat getInputFormat(IndexIngestionSpec ingestionSchema)
}
@Override
- public void cleanUp(TaskToolbox toolbox, @Nullable TaskStatus taskStatus) throws Exception
+ protected boolean shouldCleanupTask()
Review Comment:
Is this change really needed?
Is `shouldCleanupTask()` meant to be used in implementations other than `IndexTask`?
The javadoc of `Task.cleanup()` already calls out that this method is overridden only by implementations that need to perform any special cleanup. If not overridden, the default impl of `cleanup` is noop.
Adding another `shouldCleanupTask()` method in the mix seems unnecessary, esp since it is used in just one place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "adithyachakilam (via GitHub)" <gi...@apache.org>.
adithyachakilam commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1515240184
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java:
##########
@@ -116,7 +116,7 @@ public void uploadBlockBlob(final File file, final String containerName, final S
try (FileInputStream stream = new FileInputStream(file)) {
// By default this creates a Block blob, no need to use a specific Block blob client.
// We also need to urlEncode the path to handle special characters.
- blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length());
+ blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length(), true);
Review Comment:
`overwrite=true` is now not required since we would not be. pushing files multiple times now ?
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java:
##########
@@ -116,7 +116,7 @@ public void uploadBlockBlob(final File file, final String containerName, final S
try (FileInputStream stream = new FileInputStream(file)) {
// By default this creates a Block blob, no need to use a specific Block blob client.
// We also need to urlEncode the path to handle special characters.
- blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length());
+ blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length(), true);
Review Comment:
`overwrite=true` is now not required since we would not be pushing files multiple times now ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1516582177
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java:
##########
@@ -116,7 +116,7 @@ public void uploadBlockBlob(final File file, final String containerName, final S
try (FileInputStream stream = new FileInputStream(file)) {
// By default this creates a Block blob, no need to use a specific Block blob client.
// We also need to urlEncode the path to handle special characters.
- blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length());
+ blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length(), true);
Review Comment:
i wanted to bring the azure client in line with how other deep storage systems handle this like s3/google cloud
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix bug with mmless ingestion and compaction tasks on azure (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #16065:
URL: https://github.com/apache/druid/pull/16065#discussion_r1518031161
##########
extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java:
##########
@@ -116,7 +116,7 @@ public void uploadBlockBlob(final File file, final String containerName, final S
try (FileInputStream stream = new FileInputStream(file)) {
// By default this creates a Block blob, no need to use a specific Block blob client.
// We also need to urlEncode the path to handle special characters.
- blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length());
+ blobContainerClient.getBlobClient(Utility.urlEncode(blobPath)).upload(stream, file.length(), true);
Review Comment:
yeah makes sense, will add one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org