Fail query runner when nodes do not come up #25184
Open
+31
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tdcmeehan
requested review from
jaystarshot,
feilong-liu,
elharo,
ClarenceThreepwood and
a team
as code owners
prestodb-ci
requested review from
a team,
BryanCutler and
anandamideShakyan
and removed request for
a team
| (server.isResourceManager() && activeNodeCount != expectedActiveNodesForRm)) { | ||
| return false; | ||
| if (!allNodes.getInactiveNodes().isEmpty()) { | ||
| if (nanosSince(startTimeInMs).compareTo(timeout) >= 0) { |
There was a problem hiding this comment.
Nit : Abstract the checked condition with custom exception message as a lambda maybe ?
Comment on lines
+530
to
+551
| while (true) { | ||
| for (TestingPrestoServer server : servers) { | ||
| AllNodes allNodes = server.refreshNodes(); | ||
| int activeNodeCount = allNodes.getActiveNodes().size(); | ||
|
|
||
| if (!allNodes.getInactiveNodes().isEmpty() || | ||
| (server.isCoordinator() && activeNodeCount != expectedActiveNodesForCoordinator) || | ||
| (server.isResourceManager() && activeNodeCount != expectedActiveNodesForRm)) { | ||
| return false; | ||
| if (!allNodes.getInactiveNodes().isEmpty()) { | ||
| if (nanosSince(startTimeInMs).compareTo(timeout) >= 0) { | ||
| throw new TimeoutException(format("Timed out waiting for all nodes to be globally visible. Inactive nodes: %s", allNodes.getInactiveNodes())); | ||
| } | ||
| break; | ||
| } | ||
| else if ((server.isCoordinator() || server.isResourceManager()) && activeNodeCount != expectedActiveNodes) { | ||
| if (nanosSince(startTimeInMs).compareTo(timeout) >= 0) { | ||
| throw new TimeoutException(format( | ||
| "Timed out waiting for all nodes to be globally visible. Node count: %s, expected: %s", | ||
| activeNodeCount, expectedActiveNodes)); | ||
| } | ||
| break; | ||
| } | ||
| return; | ||
| } | ||
| MILLISECONDS.sleep(10); |
There was a problem hiding this comment.
Seems in the new logic, if the first TestingPrestoServer server satisfies the check, we will return immediately. Should we make sure that all the servers satisfy the check before returning?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When query runners fail to launch a cluster, a log is printed that shows this, but it doesn't fail the query runner. This could result in timeouts. It is better to fail early when the cluster does not launch.
Motivation and Context
More obvious errors.
Impact
See above
Test Plan
Query runners are used throughout our testing suite, so this should be thoroughly tested with existing tests.
Contributor checklist
Release Notes