💥 [Breaking] Asyncify slot suppliers #2433
Conversation
| * your implementation. You may want to catch it to perform any necessary cleanup, and then you | ||
| * should rethrow the exception. |
There was a problem hiding this comment.
and then you should rethrow the exception
They can't rethrow without wrapping because the checked exception was removed from this interface. We should add back throws InterruptedException, though could consider throws Exception as a way of saying we have code to handle any exception.
| @@ -32,6 +33,18 @@ public class ResourceBasedSlotSupplier<SI extends SlotInfo> implements SlotSuppl | |||
| private final ResourceBasedController resourceController; | |||
| private final ResourceBasedSlotOptions options; | |||
| private Instant lastSlotIssuedAt = Instant.EPOCH; | |||
| // For slot reservations that are waiting to re-check resource usage | |||
| private static final ScheduledExecutorService scheduler = | |||
There was a problem hiding this comment.
I think you should consider accepting this in the constructor. Can have a default, but we should allow people to control thread creation if they want.
| .orElseGet( | ||
| () -> | ||
| CompletableFuture.supplyAsync(() -> null, delayedExecutor(10)) | ||
| .thenCompose(ig -> scheduleSlotAcquisition(ctx))); |
There was a problem hiding this comment.
ChatGPT is telling me that cancellation does not propagate across orElseGet or thenCompose operators, but that doesn't seem right and I can't find this in the docs at https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html. May need a test somehow confirming cancel after this orElseGet does actually prevent the delayed call from being invoked. If it doesn't work, I can make some suggestions I think.
There was a problem hiding this comment.
I think this works fine because the returned future is permitFuture which is thenComposed with the future from here. Added a test that confirms.
There was a problem hiding this comment.
My concern is that ChatGPT says that a cancel of the outer future does not propagate the cancel to the thenComposed future. Not sure I believe it because I haven't tested, but it does concern me. I would have to test Java behavior of cancel when the thenCompose has already run to create a new future. I may set aside some time to do this.
But I fear reading https://stackoverflow.com/questions/25417881/canceling-a-completablefuture-chain and poking around, there may need to be some other mechanism to make sure the outer completable future cancel propagates to cancelling the delayed executor. I couldn't tell if the test was doing that. I wouldn't be surprised if cancel is not hierarchical.
| */ | ||
| SlotPermit reserveSlot(SlotReserveContext<SI> ctx) throws InterruptedException; | ||
| CompletableFuture<SlotPermit> reserveSlot(SlotReserveContext<SI> ctx) throws Exception; |
There was a problem hiding this comment.
| CompletableFuture<SlotPermit> reserveSlot(SlotReserveContext<SI> ctx) throws Exception; | |
| Future<SlotPermit> reserveSlot(SlotReserveContext<SI> ctx) throws Exception; |
I wonder if this is more flexible. That some implementations happen to use CompletableFuture doesn't affect us right? Or do we need CompletableFuture utilities besides tests?
There was a problem hiding this comment.
I feel like it's less flexible. You can't compose raw Futures or really do much of anything with them besides get them
What was changed
Made the
reserveSlotmethod onSlotSupplierinterface async.Note that, as it stands, this can actually slightly increase the number of threads used because the slot suppliers have been changed in a way that they will not block the caller (which is the whole point) but in order to do that, the resource based one at least needs a couple threads that it didn't before.
For the fixed size supplier, I think I've managed to come up with something that should always be non-blocking.
Why?
This is to support #1456 where the pollers themselves will be made async, and thus will be able to take advantage of async slot reservation as well.
Checklist
Closes
How was this tested:
Existing tests
Any docs updates needed?