[SYSTEMDS-3960] Fix federated worker readiness probe on delayed connections - #2587
[SYSTEMDS-3960] Fix federated worker readiness probe on delayed connections#2587gaturchenko wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2587 +/- ##
============================================
- Coverage 71.44% 71.38% -0.07%
- Complexity 50437 50532 +95
============================================
Files 1629 1632 +3
Lines 195513 196229 +716
Branches 38118 38199 +81
============================================
+ Hits 139686 140069 +383
- Misses 44867 45122 +255
- Partials 10960 11038 +78 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Waiting for the review by @ywcb00 |
ywcb00
left a comment
ywcb00
left a comment
There was a problem hiding this comment.
Thank you very much for this PR @gaturchenko. :)
I left some comments in the code. Also, could you please try to shorten the method headers and in-line documentation if possible?
Thanks and all the best, David
| * Test helpers that block until a federated worker is accepting TCP connections on its port. The federated worker opens | ||
| * its TCP port after Netty's {@code bind().sync()} returns; a successful TCP connect to that port therefore indicates |
There was a problem hiding this comment.
While removing the html paragraph tag is good, please preserve the line break.
| * @return the timeout to pass to {@link Socket#connect}, or 0 if no attempt should be made. Never returns 0 while | ||
| * time is left, because {@code connect} reads a timeout of 0 as 'infinite'. | ||
| */ | ||
| public static int attemptTimeout(long remainingMs) { |
There was a problem hiding this comment.
This name here is ambiguous. While this method returns the timeout for a connection attempt, the name could also mean that it attempts the connection with a timeout.
| private static boolean tryConnect(int port) { | ||
| private static boolean tryConnect(int port, long deadline) { | ||
| final int timeout = attemptTimeout(deadline - System.currentTimeMillis()); | ||
| if(timeout == 0) // out of time, do not start another attempt |
There was a problem hiding this comment.
This condition here can be rewritten to a "less than or equal" condition, thereby removing the necessity of the separate condition in attemptTimeout() and allowing for replacing the call to attemptTimeout() by the single remaining code line.
| private static final int TOLERATED_HANDSHAKE_MS = 1000; | ||
|
|
||
| @Test | ||
| public void attemptBudgetCoversADelayedHandshake() { |
There was a problem hiding this comment.
I appreciate verifying the code through these unit tests.
However, I think these tests are too fine-grained for testing the test framework. Please remove the tests that evaluate attemptTimeout() directly (aligning with removing of this method, as commented below), and move the remaining tests to the FederatedUrlParserTest.
github-project-automation
Bot
moved this from In Progress
to In Review
in SystemDS PR Queue
… to "less or equal" in `tryConnect()`, shorten comments
…deratedUrlParserTest`
2 participants
With any delay on the worker's interface, currently every federated test fails before it starts, while the worker is listening the whole time.
FederatedWorkerUtils.tryConnectallocated 25ms per connect attempt, but a TCP handshake needs two traversals of the link which are ~100ms under the delay above. Every attempt expired with aSocketTimeoutException, which is anIOExceptionand therefore indistinguishable from a closed port, so the probe never succeeded.The following changes were implemented:
attemptTimeout), so a slow connect cannot exceed it. The returned value of 0 implies the attempt was never madewaitForWorkersnow rechecks the deadline per port, which can now cost up to 2s each