-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-30362 Fix setSlaveAffinity regression (since 7.8) #17820
HPCC-30362 Fix setSlaveAffinity regression (since 7.8) #17820
Conversation
4a61e11
to
2494d9a
Compare
2494d9a
to
18cc762
Compare
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.
Yes, looks ok to me.
setSlaveAffinity needed to be called after the worker had registered with the manager, only then are the thor configuration settings available, and in particular slavesPerNode which setSlaveAffinity depends upon. Signed-off-by: Jake Smith <[email protected]>
18cc762
to
637576a
Compare
https://track.hpccsystems.com/browse/HPCC-30362 |
@shamser @mckellyln - please re-review. The previous version wasn't correct in the context of the cloud. |
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.
minor: (in applyResourcedCPUAffinity) If cpusI rounds down to zero, the message on line 1593 would be incorrect. (Line 1594 sets cpuI to 1 if was rounded to zero).
Agreed, but I'll leave it as it is, with this PR being a change for handling setSlaveAffinity handling, |
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.
Approved.
524e998
into
hpcc-systems:candidate-9.4.x
setSlaveAffinity needed to be called after the worker had
registered with the manager, only then are the thor
configuration settings available, and in particular
slavesPerNode which setSlaveAffinity depends upon.
Signed-off-by: Jake Smith [email protected]
Type of change:
Checklist:
Smoketest:
Testing: