You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2025/06/24 14:45:52 UTC
[PR] [WIP] Route task logs on Indexers to dedicate task log files (druid)
kfaraz opened a new pull request, #18170:
URL: https://github.com/apache/druid/pull/18170
### Description
While working on #18147 , it became apparent that it would be nice to have task logs for Indexer in separate files so that it is easier to debug them. This can also come in handy while running Indexers in general.
This PR currently has two sets of changes:
- [Done] Route the logs from the main task thread to the task log file. This is already working as verified through simulation tests by updating the `log4j2.xml` in `extensions-core/multi-stage-query/src/main/resources` and writing a simulation test in `quidem-ut`
- [WIP] Route the logs from other task threads (persist, push, publish) to the same log file. This is currently a work in progress in this PR. It requires some clean up of the appenderator API which is why all appenderator classes are being moved to the `indexing-service` module.
<hr>
This PR has:
- [ ] 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)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] 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.
- [ ] 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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz closed pull request #18170: Route task logs on Indexers to dedicated task log files
URL: https://github.com/apache/druid/pull/18170
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2190817820
##########
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:
>Yeah, I set out to do that originally. But then I realized that these threads are not reused for another task ID. They are always freshly created for each task.
ah i see what you're saying about the thread, in that case, why clear it at all? That might be less confusing, especially if there was a comment by `setTaskThreadContext` about it. clearing it sometimes is what makes it look like a mistake at a glance
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2191338727
##########
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:
Yeah, that's a good point. I have removed the `clearTaskThreadContext` method.
If we ever decide to reuse the threads, we can revisit this.
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18170:
URL: https://github.com/apache/druid/pull/18170#issuecomment-3030294118
Thanks for the review, @
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2184226572
##########
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:
Updated the javadoc for `setTaskThreadContext()`. Let me know what you think about the try-finally.
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #18170:
URL: https://github.com/apache/druid/pull/18170
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2184146665
##########
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:
Yeah, I set out to do that originally. But then I realized that these threads are not reused for another task ID. They are always freshly created for each task.
https://github.com/apache/druid/blob/00c0d57e5ca762cf8931be43350da64b8a365e1e/server/src/main/java/org/apache/druid/segment/realtime/appenderator/BatchAppenderator.java#L224-L242
https://github.com/apache/druid/blob/00c0d57e5ca762cf8931be43350da64b8a365e1e/server/src/main/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderator.java#L1246-L1268
Please let me know if you feel that adding the try-finally makes for cleaner code.
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2184160797
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/UnifiedIndexerAppenderatorsManager.java:
##########
@@ -198,7 +195,19 @@ public Appenderator createRealtimeAppenderatorForTask(
rowIngestionMeters,
parseExceptionHandler,
centralizedDatasourceSchemaConfig
- );
+ ) {
Review Comment:
Thanks! Updated.
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/UnifiedIndexerAppenderatorsManager.java:
##########
@@ -238,7 +248,19 @@ public Appenderator createBatchAppenderatorForTask(
rowIngestionMeters,
parseExceptionHandler,
centralizedDatasourceSchemaConfig
- );
+ ) {
Review Comment:
Updated.
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18170:
URL: https://github.com/apache/druid/pull/18170#issuecomment-3049296084
Thanks for the reviews, @capistrant , @clintropolis , @cryptoe !
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2191090385
##########
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:
plus one to clint's comment. I'd say we either strictly make sure it is cleared in a way that could be accurate if the threads were re-used for a different taskId, or not clear it at all
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18170:
URL: https://github.com/apache/druid/pull/18170#discussion_r2184147582
##########
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:
I guess we should still add a comment to that effect, even if we don't do the try-finally.
--
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] Route task logs on Indexers to dedicated task log files (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
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: 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