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

Added worker info logging #7

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Added worker info logging #7

merged 1 commit into from
Dec 6, 2023

Conversation

slydiman
Copy link
Collaborator

@slydiman slydiman commented Dec 1, 2023

Subj

#LLVM_LOCAL begin
log.msg(f" host: {workerinfo['host']}")
log.msg(f" admin: {workerinfo['admin']}")
log.msg(f" access_uri: {workerinfo['access_uri']}")
Copy link
Owner

Choose a reason for hiding this comment

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

If for any reason a key is missing, we would get an exception here. How about using get with default value "" or something like that?

Copy link
Collaborator Author

@slydiman slydiman Dec 5, 2023

Choose a reason for hiding this comment

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

Please look at line 444 above where workerinfo is created. It is guaranteed to have these keys

 workerinfo = {
  'admin': conn.info.get('admin'),
  'host': conn.info.get('host'),
  'access_uri': conn.info.get('access_uri'),
  'version': conn.info.get('version')
}

Also note yield self.master.data.updates.workerConnected(..., workerinfo=workerinfo) does not change workerinfo.

We can also use conn.info.get('admin') directly, but it seems more expensive.

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough. self.master.data.updates.workerConnected just writes to the database and should not change the workerinfo indeed. We can relay on that and crashing is Ok if the assumption gets broken.

@slydiman slydiman force-pushed the add-worker-info-log branch from fa2fa8f to ad11fce Compare December 5, 2023 14:08
@slydiman slydiman requested a review from gkistanova December 5, 2023 14:28
Copy link
Owner

@gkistanova gkistanova left a comment

Choose a reason for hiding this comment

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

LGTM.

#LLVM_LOCAL begin
log.msg(f" host: {workerinfo['host']}")
log.msg(f" admin: {workerinfo['admin']}")
log.msg(f" access_uri: {workerinfo['access_uri']}")
Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough. self.master.data.updates.workerConnected just writes to the database and should not change the workerinfo indeed. We can relay on that and crashing is Ok if the assumption gets broken.

@slydiman slydiman force-pushed the add-worker-info-log branch from ad11fce to 5a19da6 Compare December 5, 2023 18:55
Copy link
Collaborator

@andreil99 andreil99 left a comment

Choose a reason for hiding this comment

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

LGTM

@gkistanova gkistanova merged commit 98c7a22 into llvm/3.x Dec 6, 2023
11 checks passed
log.msg(f" system: {self.worker_system}")
log.msg(f" worker_commands: {self.worker_commands}")
log.msg(f" basedir: {self.worker_basedir}")
log.msg(f" environ: {self.worker_environ}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot to mention. Since we print out multiple lines one by one, the printout could be interleaved by other log messages from different threads. Maybe prepare the whole thing first, and then print it once? Would be much easier to read and track in the log.

gkistanova pushed a commit that referenced this pull request Jan 5, 2024
gkistanova pushed a commit that referenced this pull request Jan 9, 2024
@slydiman slydiman deleted the add-worker-info-log branch March 2, 2024 11:04
gkistanova pushed a commit that referenced this pull request Jul 7, 2024
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.

3 participants