Dennis-Mircea commented on PR #28106: URL: https://github.com/apache/flink/pull/28106#issuecomment-4621511479
> @Dennis-Mircea My AI was happy with the changes - and made some minor suggestions, I, thought I would share and see if you wanted to address them. > > Consider extracting polling utility - The waitUntilConditionWithTimeout pattern could be a reusable test utility Document timeout values - 10s and 30s timeouts are reasonable but could have brief comments explaining why Thread state handling - The !t.isAlive() check is good defensive programming Thanks @davidradl, appreciate you sharing these. 1. **Reusable polling utility** - The two polling spots already go through shared helpers: `SplitFetcherManagerTest` uses `org.apache.flink.test.util.TestUtils.waitUntil`, and `RescaleTimelineITCase.waitUntilConditionWithTimeout` is a pre-existing private wrapper around `CommonTestUtils.waitUntilCondition` (this PR only adds a call to it). So there's no new duplication introduced here. Promoting that wrapper into a shared util touches a broadly-used test utility and is really a separate refactor. I'd keep it out of this `[hotfix]` to honor the "separate cleanup from functional changes" convention, and can file a follow-up JIRA if it's wanted. 2. **Document timeout values** - The 30s wait in `SplitFetcherManagerTest` already got an explanatory comment. I've now added a brief note on the new 10s timeout in `RescaleTimelineITCase` explaining why it's a generous upper bound rather than a load-bearing value. 3. **`!t.isAlive()` check** - Thanks, glad that read well. -- 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]
