lib: refactored cluster schedule_handlers #32485
Conversation
nodejs-github-bot
added
the
cluster
| class SharedHandle { | ||
| constructor(key, address, { port, addressType, fd, flags }) { | ||
| this.key = key; | ||
| this.workers = new Map(); |
There was a problem hiding this comment.
Wouldn't a WeakSet (or SafeWeakSet) be a better fit than Map here?
There was a problem hiding this comment.
Since in map we are relying on a key, that is in our case is id. On if deep-clone an object itself and pass it then also the lookup will not be evaluated to true. Ref:
> a = {b: 'some key'};
{ b: 'some key' }
> st = new WeakSet()
WeakSet { <items unknown> }
> st.add(a)
WeakSet { <items unknown> }
> st
WeakSet { <items unknown> }
> m = Object.assign({}, a)
{ b: 'some key' }
> st.has(m)
false
> st.has(a)
true
>
| this.workers = new Map(); | ||
| this.handle = null; | ||
| this.errno = 0; | ||
| const rval = this.determineRval(addressType, |
There was a problem hiding this comment.
This method is only used here, I don't think it's worth moving to a separate function. I think keeping the original conditionals here is best.
There was a problem hiding this comment.
The previous implement was using let and I think if we can restructure the logic to const that solidifies the meaning of that variable. Let's say if one goes from top to bottom reading this file, then reaching this rvalue variable one can infer that it will be a fixed rval determined by the function.
There was a problem hiding this comment.
I don't see the use of let being a problem, especially given the size of the function and that the assignments happen immediately after the declaration.
There was a problem hiding this comment.
let send a wrong signal on the meaning of variable as it can be changed, but const signifies that the value will not be changed and thus can be believed to be the same throughout the scope of the class.
| this.handles = []; | ||
| this.handle = null; | ||
| this.server = net.createServer(assert.fail); | ||
| this.attachListener(fd, port, address, flags); // UNIX socket path. |
There was a problem hiding this comment.
Since this method is only used here, I think it's best to just keep the original code here.
There was a problem hiding this comment.
Isn't now it is giving it much more meaning to what the block is actually doing, and also if there are more case logic in some block, better to abstract it in some other place. This is done because in the future also there might be some more conditionals around that code.
There was a problem hiding this comment.
I think if the logic gets that complicated then we can revisit it at that point. Separating out 10 lines of code does not seem worthwhile.
There was a problem hiding this comment.
I still think that decreasing the vertical complexity gives the code a nice look and meaningfulness. I am still not clear about why moving it to previous implementation gives it verbosity and meaningfulness.
| return; // Worker is closing (or has closed) the server. | ||
| distribute(err, handle) { | ||
| this.handles.push(handle); | ||
| const worker = this.free.values().next().value; |
There was a problem hiding this comment.
The previous implementation actually removed the worker from .free(), this will keep the worker in place.
There was a problem hiding this comment.
Can get the key and remove it from the map instead.
| constructor(key, address, { port, fd, flags }) { | ||
| this.key = key; | ||
| this.all = new Map(); | ||
| this.free = new Map(); |
There was a problem hiding this comment.
I think it's best to keep this as an array since an array is most likely designed better for the purposes of a queue than a Map is. The only real benefit of the Map is faster lookups, but we shouldn't really optimize that over faster/easier shift()/push() because workers are not likely to be added/removed as often as workers being selected for requests.
There was a problem hiding this comment.
True, but shifting logic is at the time of distributing the load and that is a single operation that can be implemented using keys iterator on map is still an O(1) operation and getting the element and deleting that from the Hash is same on average. Reimplemented to delete the worker from free pool.
yashLadha
force-pushed
the
refactor/worker_scheduling_handlers
branch
from
20c71b8
to
2b6b006
Compare
Scheduler handler for cluster were implemented in function prototype form, which can be converted to newer syntax of classes. Also seperated concern for attaching listener to one of the internal methods of class in round_robin_handle. Used `Map` instead of `Array` when there are cases for continous finding element. Fixes: nodejs#32480
yashLadha
force-pushed
the
refactor/worker_scheduling_handlers
branch
from
2b6b006
to
e6c9a26
Compare
|
This PR contains a single commit that does two unrelated things:
Don't do that - it makes review and back-porting difficult. Start with (1) and open a new PR for (2) after (1) lands. Aside: I see a couple of long lines in there. I'm not sure why the linter isn't complaining but lines should stay <= 80 columns. My personal opinion: (1) is a lot of code churn for little to no benefit. It doesn't make a material difference for readability or maintainability and the current code is working just fine. Leave well enough alone. |
|
@bnoordhuis I can split the functional changes into a new PR. |
|
Closing this cosmetic and functional changes are mixed into one commit. |
Scheduler handler for cluster were implemented in function prototype
form, which can be converted to newer syntax of classes. Also seperated
concern for attaching listener to one of the internal methods of class
in round_robin_handle. Used
Mapinstead ofArraywhen there arecases for continous finding element. Also prefered destructuring over
sending large number of parameters because one of the parameters
addressTypecan only be used in shared_handle implementation and notround_robin.
Fixes: #32480
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes