[CDAP-20683] Propogate logs from System Worker to Pipeline Logs #15284
Conversation
dj-smart
changed the title
| @@ -138,7 +138,7 @@ public final RuntimeInfo run(ProgramDescriptor programDescriptor, ProgramOptions | |||
| } catch (Exception e) { | |||
| controller.failed(e); | |||
| programStateWriter.error(programRunId, e); | |||
| LOG.error("Exception while trying to run program run {}", programRunId, e); | |||
| LOG.error("Exception while trying to run program with runID {}", programRunId, e); | |||
There was a problem hiding this comment.
Why do you need this change? program run is a proper term
There was a problem hiding this comment.
Ok, I misinterpreted term "program run", so I changed it when I came across it. Will revert it back.
| @@ -649,6 +653,7 @@ public TwillController create(@Nullable Callable<Void> startupTask, long timeout | |||
| } | |||
| programStateWriter.error(programRunId, throwable); | |||
| } | |||
| restoreContext.cancel(); | |||
There was a problem hiding this comment.
Please use try..finally to ensure context is always properly cancelled, even if exception is thrown out.
BTW: It's a perfect case for try with resources. We just need to either make Cancellable extend AutoClosable or at least setLoggingContext return an AutoClosable result
There was a problem hiding this comment.
Refactored the code to use try-with-resources. PTAL.
| * @param loggingContext context to set | ||
| * @return Autocloseable that can be used to revert the logging context and MDC Map to its original | ||
| */ | ||
| public static AutoCloseable restoreLoggingContext(LoggingContext loggingContext) { |
There was a problem hiding this comment.
This looks strange. Usually try with resource should return a resource. Any cleanup happens on exit of the block. My recommendation: is to create an interface that implements both Cancelable and AutoClosable and make the setLoggingContext return it
There was a problem hiding this comment.
I have made changes based on your suggestion. Let me know if this is in line with the expectation.
cdap-common/src/main/java/io/cdap/cdap/common/logging/LoggingContextAccessor.java
Show resolved
Hide resolved
cdap-common/src/main/java/io/cdap/cdap/common/logging/LoggingContextAccessor.java
Show resolved
Hide resolved
JIRA: CDAP-20683
The StartupTask timeout logs have been propagated to pipeline logs, as can be seen in this image:
