facebook / buck2

Build system, successor to Buck
https://buck2.build/
Apache License 2.0
3.37k stars 199 forks source link

Remove arbitrary concurrency limiter for uplods and replace with a configurable limit that is disabled by default. #642

Open TheGrizzlyDev opened 1 month ago

TheGrizzlyDev commented 1 month ago

The existing limit doesn't really help to achieve the goals it states as it cannot help in conditions where multiple clients are using RE at the same time, which are the common exercise conditions. Since the flag can be used to limit quickly uploads in case of some issues with RBE I opten in for keeping it in some form, by making it a setting, though it likely isn't very useful, so it's disabled by default. Instead we should rely on other control mechanisms like number of connections and TCP or HTTP/2 flow control or, like in this case, explicit errors coming from the RE protocol itself (like RESOURCE_EXHAUSTED or others).

JakobDegen commented 1 month ago

cc @benbrittain who originally added this

The existing limit doesn't really help to achieve the goals it states as it cannot help in conditions where multiple clients are using RE at the same time, which are the common exercise conditions

Yeah, but even if they're the common exercise conditions, it's still pretty easy to imagine that they might not be the failure conditions. Ie you could imagine some server that has one thread per client or something like that and falls over if a client shows up trying to write too many big files at once.

I think there's a nice in-between option to go for though, which both preserves this property and sounds closer to what you actually want: Can we only apply this concurrency to the non-batched uploads, and let batched uploads proceed at unlimited concurrency?

TheGrizzlyDev commented 1 month ago

Yeah, but even if they're the common exercise conditions, it's still pretty easy to imagine that they might not be the failure conditions.

Which is why there is an optional configuration to limit concurrency. Any default value would be making likely incorrect assumptions about the implementation and exercise conditions of a RE server and this is just wrong as there are multiple implementations and all of them are wildly different and can run on extremely large distributed clusters with a wide array of different hardware and software specifications. This invariably results in lower performances by default with no real impact. On the other hand if such a limit is necessary for a RE server they can state this setting in their docs. I think this is already a middle ground and brings the potential benefit of limiing concurrency whilst keeping a performant setup by default, wdyt?

TheGrizzlyDev commented 1 month ago

friendly ping @JakobDegen :)

TheGrizzlyDev commented 2 weeks ago

Ping :)

valadez-abel commented 1 week ago

I tested this on our RE setup, and without it the client seemed to be stuck for 40+ minutes on re_upload stages, but with this PR, it's no longer blocked on re_upload.

zjturner commented 1 week ago

Agree that it would be great to see some forward progress on this. @JakobDegen is there anything you're waiting for?