Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Scheduling Topology Hints #180
Add Scheduling Topology Hints #180
Changes from 2 commits
e3bc10c
b9c12e5
0402cef
850801e
941804e
1a91cfc
79862ab
93fedb7
a325bc6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The need for this function isn't immediately obvious and it feels like a bit of a hack. In general if an API needs to be changed to support a test you have one of two things going on: (i) your test is too invasive and checking too much internal logic; (ii) the API doesn't expose enough information.
In this case I think it's (i). I think it can be changed relatively easily as this and
callFunctions
have the same signature. It might be possible to change the tests to callcallFunctions
instead (as they have mock mode turned on). You can then add a check to make sure the underlying function calls have been disaptched to the expected hosts.If this isn't possible, then we need to work out what's happening in
callFunctions
that doesn't work properly in mock mode.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, switching to
callFunctions
is pretty easy with the only caveat that we need to use theTestExecutor
andTestExecutorFactory
classes that currently lived in./tests/tests/scheduler/test_executor.cpp
.I have moved the declaration of these classes to
fixtures.h
and kept the definition where it is.I also add a check for the recorded messages in the function call client.
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.
My gut here would instead be:
I think this fixes the issue you mention in the PR description too.
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.
I don't think this would behave as expected. For example:
NEVER_ALONE
hint).However, I think your solution would schedule 5 requests on the first host and 4 on the second, as it would exhaust all possible hosts (
nOnThisHost == 1
for all of them), and resort to overload the master.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.
After an offline discussion, I use this change together with a change in the overloading logic that makes the issue mentioned in the description disappear.