Dzeri96 commented on PR #3598: URL: https://github.com/apache/celeborn/pull/3598#issuecomment-3872570498
I can can't really review this code, but I can give my two cents: 1. Parallelizing so high in the call chain makes it hard to know if the called code will produce any race conditions. Since we are in the controller, I guess the method can be called asynchronously anyway, so you won't break anything that isn't already broken, but I think it would be better to encapsulate only the time-sensitive part of the code and return a Future from that function, rather than parallelizing everything. 3. Maybe factor out the duplicate code block that assigns primary and replica slots? 5. I don't know enough to be sure, but it seems to me that this method should maybe queue up requests instead of executing them greedily? If the worker gets many `handleReserveSlots` requests, they might fight with each-other and in the end they could all fail? I guess the master should prevent this, but I'm not sure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
