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.
I was thinking a little more about how #8430 could be used and ended up wanting to use this for the client as well.
(This is also loosely motivated by dask/dask-expr#765 where I suggested to move the key generation / tokenization from client to scheduler where the ordered sendrcv of #8430 would be nice)
The client unfortunately re-implements the RPC stream handler of the server base class making it hard to reuse the ordered sendrcv without a lot of code duplication.
This PR suggests a refactoring where I propose to move from inheritance to composition regarding the
Server
networking and RPC logic. The currentServer
base class is mixing a lot of application logic with networking logic which in the past frequently caused issues during teardown so separation of those concerns is long overdue.The
Server
as proposed in this PR should likely rather be calledRPCServer
orRPCSomethingElse
since it doesn't actually implement a traditional server<->client model but I'm trying to not get hung up on naming at this point.With this decomposition I was then able to use the slimmed down
Server
in the client as well which opens up the reuse of smth like #8430This refactoring is not really done but if we go down this road, the universally used
ConnectionPool
andstream_comms
would basically blend into this single object which would support three ways to communicateState of this PR: Mostly working but still a couple of failing tests. From what I saw nothing insurmountable.
There are two commits
I think I'm mostly interested in feedback about the first one since that may have merit without the introduction of new features