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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
