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

JobActor::WorkerManager's logs contain the WorkerID that they correspond to #411

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

Conversation

GautamGottipati
Copy link

Context

Explain context and other details for this pull request.

Checklist

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

@@ -1286,6 +1288,8 @@ class WorkerManager implements IWorkerManager {
.build();
private volatile boolean stageAssignmentPotentiallyChanged;

private WorkerManagerLogger workerLogger;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Introducing extra lifecycle for these loggers doesn't seem to provide enough benefits. Adding the extra metadata to the logger message is sufficient enough.

Copy link
Author

@GautamGottipati GautamGottipati Apr 12, 2023

Choose a reason for hiding this comment

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

Hi @Andyz26 can you please give me some hints, like what metadata I could add and how. In the issue it was mentioned to implement something like this workerManager.info("Whatever"); where by default workerId should be added. Taking reference from issue #384

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue is about jobId at the job actor level and that is a different structure comparing to this workerManager. Here WorkerManager is not 1 to 1 mapping with a specific worker logically. There is no need to maintain a worker-level logger as long as the information needed is logged.

Copy link
Author

Choose a reason for hiding this comment

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

so you mean, I can directly add worker Id to Logger.info, where ever worker id is not mentioned.

@Andyz26
Copy link
Collaborator

Andyz26 commented Apr 12, 2023 via email

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