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