-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CDAP-20683] Propogate logs from System Worker to Pipeline Logs #15284
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this change? program run is a proper term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made changes based on your suggestion. Let me know if this is in line with the expectation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with small comments
JIRA: CDAP-20683
The StartupTask timeout logs have been propagated to pipeline logs, as can be seen in this image: