patricklucas commented on PR #22987:
URL: https://github.com/apache/flink/pull/22987#issuecomment-1651315710

   @XComp Made the following changes:
   - Updated `testCloseClientWhileProcessingRequest` to check that 
`responseChannelFutures` is empty before and size 1 after the call to 
`sendRequest`
   - Added a test that preloads a response channel future and makes sure it's 
resolved as expected
   - To accomplish that, added `@VisibleForTesting 
RestClient#getResponseChannelFutures()`
   - To keep consistency, I changed one other field which was 
`@VisibleForTesting`, but non-final and without a getter, hope this is okay
     - Making the field final should be a strict improvement, but being 
consistent about adding a getter rather than exposing a bare field also seems 
good
     - I also made a symmetric change in `RestServerEndpoint`—the diff should 
be clear as to why that made sense
   
   It occurred to me that there is actually no test in `RestClientTest` that 
actually tests the happy path. If there were, I would have copied it to test 
that the response channel future is added and also cleared when the client is 
not closed, but the request completes successfully. I'm not super motivated to 
do that right now as I'm very busy and have already put a good bit of time into 
this change—what do you think?


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to