Skip to content

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

.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