Skip to content
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

Ensure JobActor has JobID as part of every emitted log line #384 #408

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

7ue5wu
Copy link

@7ue5wu 7ue5wu commented Apr 4, 2023

Context

Logger adds the jobID to every emitted logline by default.

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable

@7ue5wu 7ue5wu had a problem deploying to Integrate Pull Request April 4, 2023 02:08 — with GitHub Actions Failure
@@ -600,6 +601,7 @@ private Receive buildInitializedBehavior() {
// UNEXPECTED MESSAGES END //
.match(Terminated.class, this::onTerminated)
.matchAny(x -> {
MDC.put("name", name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this make assumptions about the thread context? Will these assumptions be held for Akka actors?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't consider it. So does it mean it is better to use ActorContext.getLog() to obtain the logger and clear the MDC after processing the current message?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to use MDC for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems I thought it the wrong way. Thank you for telling me and I will fix it after my busy final.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants