FrankChen021 commented on code in PR #19650:
URL: https://github.com/apache/druid/pull/19650#discussion_r3698986683
##########
server/src/main/java/org/apache/druid/segment/loading/OmniDataSegmentKiller.java:
##########
@@ -97,27 +103,26 @@ private DataSegmentKiller getKiller(DataSegment segment)
throws SegmentLoadingEx
@Override
public void killAll()
{
+ // Do not invoke killAll() for cloud-specific DataSegmentKiller
implementations
+ // as this is a potentially dangerous operation
throw new UnsupportedOperationException("not implemented");
}
@Override
public void killRecursively(String relativePath) throws IOException
{
- IOException firstFailure = null;
- for (Supplier<DataSegmentKiller> supplier : killers.values()) {
- try {
- supplier.get().killRecursively(relativePath);
- }
- catch (IOException e) {
- if (firstFailure == null) {
- firstFailure = e;
- } else {
- firstFailure.addSuppressed(e);
- }
- }
- }
- if (firstFailure != null) {
- throw firstFailure;
+ // Delegate kill to the currently bound deep storage implementation only.
+ // It is unnecessary and also potentially unsafe to kill all files under
the
+ // same folder name on inactive deep storage bindings provided by other
extensions.
+ final Supplier<DataSegmentKiller> killer = killers.get(deepStorageType);
Review Comment:
Thanks — the added comments explain the mismatch, but they do not avoid it.
For `druid.storage.type=s3` or `oss`, this still looks up `killers.get("s3")`
or `killers.get("oss")` while the map keys are `s3_zip` and `oss_zip`, so it
throws before reaching those killers' default no-op `killRecursively()`. The
supervisor catches the exception, but every cleanup will log a warning. Could
this explicitly no-op unsupported storage types, or map/alias the configured
storage type to the killer key?
Reviewed 17 of 17 changed files.
##########
indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/DeepStorageIntermediaryDataManager.java:
##########
@@ -49,10 +52,22 @@ public static String retrieveShuffleDataStoragePath(String
supervisorTaskId)
return SHUFFLE_DATA_DIR_PREFIX + "/" + supervisorTaskId;
}
+ /**
+ * Used by Guice to create an instance of {@link
DeepStorageIntermediaryDataManager}.
+ *
+ * @param dataSegmentPusher Always non-null
+ * @param dataSegmentKiller Can be null in certain cases such as on
MiddleManagers
+ * when using druid.storage.type=s3 since the
respective
+ * S3DataSegmentKiller uses scheme "s3_zip" instead
of "s3"
+ */
@Inject
- public DeepStorageIntermediaryDataManager(DataSegmentPusher
dataSegmentPusher)
+ public DeepStorageIntermediaryDataManager(
+ DataSegmentPusher dataSegmentPusher,
+ @Nullable DataSegmentKiller dataSegmentKiller
Review Comment:
[P1] @Nullable does not make the killer binding optional
`@Nullable` only permits an injected provider to return null; it does not
turn a failing PolyBind choice into an optional dependency. On MiddleManager or
Overlord with deep-store shuffle and `druid.storage.type=s3` or `oss`, the
`DataSegmentKiller` choice requests `s3` or `oss`, while the registered keys
are `s3_zip` or `oss_zip`, so `ConfiggedProvider` throws `ProvisionException`
while constructing this manager, before the null guard in `deletePartitions`.
This prevents the deep-store manager from being provisioned; use an actually
optional binding/provider or add storage-type aliases.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
