Skip to content

Expose ?master_timeout on get-shutdown API#108886

Merged
elasticsearchmachine merged 10 commits intoelastic:mainfrom
DaveCTurner:2024/05/22/get-shutdown-timeout
May 23, 2024
Merged

Expose ?master_timeout on get-shutdown API#108886
elasticsearchmachine merged 10 commits intoelastic:mainfrom
DaveCTurner:2024/05/22/get-shutdown-timeout

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

Relates #107862
Relates #107984

@DaveCTurner DaveCTurner added >bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown v8.15.0 labels May 22, 2024
@DaveCTurner DaveCTurner requested a review from thecoop May 22, 2024 08:07
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label May 22, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @DaveCTurner, I've created a changelog YAML for you.

}

Request getShutdownStatus = new Request("GET", "_nodes/shutdown");
if (Set.of(Version.CURRENT.toString()).equals(nodesVersions) && randomBoolean()) {
Copy link
Copy Markdown
Member Author

@DaveCTurner DaveCTurner May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good candidate for the new capabilities feature here but I couldn't work out how to use it robustly. AFAICT ESRestTestCase#clusterHasCapability will throw an exception if the handling node doesn't even support the GET _capabilities API.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like a bug @DaveCTurner, the behavior should match this:

public Optional<Boolean> clusterHasCapabilities(String method, String path, String parametersString, String capabilitiesString) {
Map<String, String> params = Maps.newMapWithExpectedSize(5);
params.put("method", method);
params.put("path", path);
if (Strings.hasLength(parametersString)) {
params.put("parameters", parametersString);
}
if (Strings.hasLength(capabilitiesString)) {
params.put("capabilities", capabilitiesString);
}
params.put("error_trace", "false"); // disable error trace
try {
ClientYamlTestResponse resp = callApi("capabilities", params, emptyList(), emptyMap());
// anything other than 200 should result in an exception, handled below
assert resp.getStatusCode() == 200 : "Unknown response code " + resp.getStatusCode();
return Optional.ofNullable(resp.evaluate("supported"));
} catch (ClientYamlTestResponseException responseException) {
if (responseException.getRestTestResponse().getStatusCode() / 100 == 4) {
return Optional.empty(); // we don't know, the capabilities API is unsupported
}
throw new UncheckedIOException(responseException);
} catch (IOException ioException) {
throw new UncheckedIOException(ioException);
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see yes that'd make more sense.

The 4-valued Optional<Boolean> is an awful return type here btw :) Doubly so since Optional.of(null) is impossible but Optional.empty() means two different things: if we get a response with a supported: null then that seems like it should be a test failure, whereas a 4xx error should just be false.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I take that back, I see some point in distinguishing "don't know" from "definitely not supported". But maybe we could just return a nullable Boolean? Some Javadocs about the different return values would be very helpful here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, depending on if you require a capability or reject a capability you might want to treat that differently.
However, we still have an ongoing discussion on that point. Rejecting a capability isn't necessarily working as one might expect at the moment, despite returning false you might still randomly hit a node that DOES support the capability, TBD (cc @thecoop )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case it will return false indicating the capability is not supported.

This is a difference in behaviour - it could return #of(false) or #empty() depending on which node it hits...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention with Optional, of course, is to never return null. So it's 3-valued as it is the case for a nullable Boolean. But 💯 on the Javadocs, I'll update these in #108889 as well 👍

@DaveCTurner DaveCTurner requested a review from mosche May 22, 2024 13:11
Copy link
Copy Markdown
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dave, LGTM 👍

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 23, 2024
@elasticsearchmachine elasticsearchmachine merged commit 4878509 into elastic:main May 23, 2024
@DaveCTurner DaveCTurner deleted the 2024/05/22/get-shutdown-timeout branch May 23, 2024 08:02
@DaveCTurner DaveCTurner restored the 2024/05/22/get-shutdown-timeout branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown Team:Core/Infra Meta label for core/infra team v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants