-
-
Notifications
You must be signed in to change notification settings - Fork 748
Active Memory Manager to use bulk comms #5357
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
Conversation
distributed/active_memory_manager.py
Outdated
| { | ||
| "op": "acquire-replicas", | ||
| "keys": [ts.key for ts in ts_to_who_has], | ||
| "stimulus_id": "acquire-replicas-" + stimulus_id, |
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.
@fjetter unsure if, when a function generates as burst of messages, the stimulus ID is supposed to be unique to each message or not?
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.
TLDR Your code change aligns with my idea
We're up to define this how we want since this system is new. I'd like think of this as "one decision creates one ID". If one decision triggers multiple messages, I would probably put the same ID into the messages. My idea about this ID is to trace the impact of a decision. In this example, a run_once call is the stimulus/trigger and makes the decision to change something about the cluster state (this breaks into many smaller decisions but they have the same origin/root cause/base state). This specific run_once call will have an ID and we'll assign this ID to all messages generated by it.
|
All failures unrelated; ready for review and merge. |
|
|
||
|
|
||
| @pytest.mark.xfail(reason="distributed#5046, distributed#5265") | ||
| @pytest.mark.xfail(reason="distributed#5265") |
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.
See #5356
fjetter
left a comment
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.
In which order should we merge? #5356
|
Happy to have yours first. This one will then need to change to remove the scheduler-side replica. |
|
Added scheduler-side instant drop of the replica post #5356. |
|
test_drop_stess fails. Need to investigate... |
23a5dc7 to
5d61001
Compare
|
This is ready to be reviewed again |
|
@fjetter are you happy to merge this? |
CC @fjetter @mrocklin @jrbourbeau