Expose metrics for pending acquire operation latency
#178
Conversation
| @@ -434,15 +440,22 @@ public void request(long n) { | |||
| /** | |||
| * Stop the countdown started when calling {@link AbstractPool#doAcquire(Borrower)}. | |||
| */ | |||
| void stopPendingCountdown() { | |||
| void stopPendingCountdown(boolean success) { | |||
| if (!timeoutTask.isDisposed()) { | |||
There was a problem hiding this comment.
Assuming timeoutTask was disposed, what should happen with the pending metric?
There was a problem hiding this comment.
Isn't the pending metric per connection pool?
There was a problem hiding this comment.
Just trying to understand - so if a borrower was just timed out, its result can be ignored, while the acquisition will actually account for recording the metric. Do I understand it right?
There was a problem hiding this comment.
!timeoutTask.isDisposed() this check is to ensure that if you invoke this method twice for one and the same Borrower you will not record twice.
There was a problem hiding this comment.
I mean timeoutTask is disposed here in this method at the end of the method, so we need to guarantee that we record just once.
There was a problem hiding this comment.
If the task timed out the recording will happen in run method. Is that what you had in mind?
There was a problem hiding this comment.
I now see what is happening. In another comment I expressed my concern about it. Now my question would be - is there no race here? Is the timeoutTask delivered serially with the acquisition or does it run on potentially another thread? In such case, perhaps the flip of the Borrower's AtomicBoolean state would be the trigger to account for either success or failure in the transitioning from pending state?
| @@ -366,7 +366,7 @@ private void drainLoop() { | |||
| borrower.fail(new PoolShutdownException()); | |||
| return; | |||
| } | |||
| borrower.stopPendingCountdown(); | |||
| borrower.stopPendingCountdown(true); | |||
There was a problem hiding this comment.
This is actually followed by borrower.deliver which calls stopPendingCountdown. Why not rely on the one in deliver then? Is it because of the re-scheduling onto the acquisitionScheduler? Perhaps there should be an abstraction for signalling that pending state is over and what the next step is so that it is encapsulated? It feels unwieldy that the logic is spread over two classes and feels error-prone. Shouldn't this actually cancel the timeout timer instead?
|
Thank you all for the review! |
This is related to reactor/reactor-netty#2946