clintropolis commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2183477476
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/BatchAppenderator.java:
##########
@@ -738,6 +739,8 @@ public ListenableFuture<SegmentsAndCommitMetadata> push(
log.info("Push done: total sinks merged[%d], total hydrants
merged[%d]",
identifiers.size(), totalHydrantsMerged
);
+
+ clearTaskThreadContext();
Review Comment:
same comment about clearing context if exceptions are thrown before we get
here
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/BatchAppenderator.java:
##########
@@ -634,6 +634,7 @@ public ListenableFuture<Object> persistAll(@Nullable final
Committer committer)
numPersistedRows, bytesPersisted, persistMillis
);
log.info("Persist is done.");
+ clearTaskThreadContext();
Review Comment:
it looks like a handful of places can throw after we call
`setTaskThreadContext()`, but before the `try`/`finally` that calls
`clearTaskThreadContext()`,
https://github.com/apache/druid/pull/18170/files#diff-58fe6f50cfefc3997a365d9d92d8f183c84b2f7fa945db548a6ea74425a159faL572
and
https://github.com/apache/druid/pull/18170/files#diff-58fe6f50cfefc3997a365d9d92d8f183c84b2f7fa945db548a6ea74425a159faR580
at least (didnt' look too closely). I think we would need to call
`clearTaskThreadContext()` there too, yes? Should we just put the whole block
under another try/finally and move the call to clear context to the outer one?
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/UnifiedIndexerAppenderatorsManager.java:
##########
@@ -238,7 +248,19 @@ public Appenderator createBatchAppenderatorForTask(
rowIngestionMeters,
parseExceptionHandler,
centralizedDatasourceSchemaConfig
- );
+ ) {
Review Comment:
nit: same thing about `{`
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/UnifiedIndexerAppenderatorsManager.java:
##########
@@ -198,7 +195,19 @@ public Appenderator createRealtimeAppenderatorForTask(
rowIngestionMeters,
parseExceptionHandler,
centralizedDatasourceSchemaConfig
- );
+ ) {
Review Comment:
nit: i think `{` should be on new line?
--
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]
