-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LLClient: Support host selection #30523
Conversation
Allows users of the Low Level REST client to specify which hosts a request should be run on. They implement the `NodeSelector` interface or reuse a built in selector like `NOT_MASTER_ONLY` to chose which nodes are valid. Using it looks like: ``` Request request = new Request("POST", "/foo/_search"); request.setNodeSelector(NodeSelector.NOT_MASTER_ONLY); ... ``` This introduces a new `Node` object which contains a `HttpHost` and the metadata about the host. At this point that metadata is just `version` and `roles` but I plan to add node attributes in a followup. The canonical way to **get** this metadata is to use the `Sniffer` to pull the information from the Elasticsearch cluster. I've marked this as "breaking-java" because it breaks custom implementations of `HostsSniffer` by renaming the interface to `NodesSniffer` and by changing it from returning a `List<HttpHost>` to a `List<Node>`. It *shouldn't* break anyone else though. Because we expect to find it useful, this also implements `host_selector` support to `do` statements in the yaml tests. Using it looks a little like: ``` --- "example test": - skip: features: host_selector - do: host_selector: version: " - 7.0.0" # same syntax as skip apiname: something: true ``` The `do` section parses the `version` string into a host selector that uses the same version comparison logic as the `skip` section. When the `do` section is executed it passed the off to the `RestClient`, using the `ElasticsearchHostsSniffer` to sniff the required metadata. The idea is to use this in mixed version tests to target a specific version of Elasticsearch so we can be sure about the deprecation logging though we don't currently have any examples that need it. We do, however, have at least one open pull request that requires something like this to properly test it. Closes elastic#21888 (kind of, it isn't in the high level client, but we'll do that in a followup)
Pinging @elastic/es-core-infra |
* first and then running the "left" selector on the results of the "right" | ||
* selector. | ||
*/ | ||
class Compose implements NodeSelector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transferring a comment by @javanna from #29211:
I am not sure that we still need this given that users can provide metadata together with the corresponding host. Can we remove this?
Transferring my reply:
We still use this in the yaml testing framework to look up metadata.
Replying to my reply:
I'm going to move this to the yaml testing framework. I buy the argument that we may not need this ever and in that case I don't want to make it and commit to supporting it. If folks file an issue about it we can bring it back over.
* if we don't know what roles the node has. | ||
*/ | ||
private final Roles roles; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add node attributes to this in a followup. Folks can tag the elasticsearch node with the rack/row/availability zone that it is in and then use a NodeSelector
to target nodes in the same rack/row/availability zone as the application server. It is a traditional elasticsearch feature and it is pretty sweet but I don't want to add yet more to this already very large PR.
OK @javanna! This is ready for you to have a look when you have a chance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first round and left a few comments, I will go deeper in the next round.
* An implementation that sorts list consistently will consistently send | ||
* requests to s single node, overloading it. So implementations that | ||
* reorder the list should take the original order into account | ||
* <strong>somehow</strong>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
food for thoughts: If this is dangerous, shall we not document it, or not allow it? Is it useful to reorder nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You argued strongly a few months ago to allow it, mostly on the grounds that something like rack awareness features would want to sort them like <on_the_rack>, <off_the_rack>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. that's what happens. I forget things. I wouldn't say I argued strongly about ordering necessarily, I just wanted the selector to get a list of all the node candidates rather than a single node. That way it can decide what to do and act as a preference, meaning if there's not nodes in the rack, it's ok to go to an alive node in another rack rather than trying to revive a node. That can be achieved without allowing the selector to mess with ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like this:
diff --git a/client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java b/client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java
index e9fbc9b6999..273cecac7ef 100644
--- a/client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java
+++ b/client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java
@@ -54,15 +54,26 @@ public interface NodeSelector {
* selector approves of, in the order that the selector would prefer
* to use them.
*/
- List<Node> select(List<Node> nodes);
+ Selector prepare(List<Node> nodes);
+
+ interface Selector {
+ boolean select(Node node);
+ }
/**
* Selector that matches any node.
*/
NodeSelector ANY = new NodeSelector() {
+ private final Selector SELECTOR = new Selector() {
+ @Override
+ public boolean select(Node node) {
+ return true;
+ }
+ };
+
@Override
- public List<Node> select(List<Node> nodes) {
- return nodes;
+ public Selector select(List<Node> nodes) {
+ return SELECTOR;
}
@Override
@@ -77,16 +88,19 @@ public interface NodeSelector {
* role. It does not reorder the nodes sent to it.
*/
NodeSelector NOT_MASTER_ONLY = new NodeSelector() {
- @Override
- public List<Node> select(List<Node> nodes) {
- List<Node> subset = new ArrayList<>(nodes.size());
- for (Node node : nodes) {
- if (node.getRoles() == null) continue;
- if (false == node.getRoles().isMasterEligible() || node.getRoles().isData()) {
- subset.add(node);
+ private final Selector SELECTOR = new Selector() {
+ @Override
+ public boolean select(Node node) {
+ if (node.getRoles() == null) {
+ return false;
}
+ return false == node.getRoles().isMasterEligible() || node.getRoles().isData();
}
- return subset;
+ };
+
+ @Override
+ public Selector select(List<Node> nodes) {
+ return SELECTOR;
}
@Override
diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java
index 1e6a477e215..d3c613c5535 100644
--- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java
+++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java
@@ -648,9 +648,14 @@ public class RestClient implements Closeable {
*/
// TODO this is going to send more requests to nodes right *after* a node that the selector removes
Collections.rotate(livingNodes, lastNodeIndex.getAndIncrement());
- List<Node> selectedLivingNodes = nodeSelector.select(livingNodes);
- if (false == selectedLivingNodes.isEmpty()) {
- return selectedLivingNodes;
+ Selector selector = nodeSelector.selector(livingNodes);
+ for (Iterator<Node> itr = livingNodes.iterator(); itr.hasNet();) {
+ if (false == selector.select(itr.next())) {
+ itr.remove();
+ }
+ }
+ if (false == livingNodes.isEmpty()) {
+ return livingNodes;
}
}
@@ -677,7 +682,12 @@ public class RestClient implements Closeable {
for (DeadNodeAndRevival n : deadNodes) {
selectedDeadNodes.add(n.node);
}
- selectedDeadNodes = nodeSelector.select(selectedDeadNodes);
+ Selector selector = nodeSelector.selector(selectedDeadNodes);
+ for (Iterator<Node> itr = selectedDeadNodes.iterator(); itr.hasNet();) {
+ if (false == selector.select(itr.next())) {
+ itr.remove();
+ }
+ }
The NodeSelector can still look at all the nodes before it makes a decision but it can't reorder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not love that two interfaces need to be implemented, especially the select
method that returns a Selector
feels a bit of a weird API to me. What do you think of accepting and returning a Collection
or a Set
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a Set
is pretty heavy. What if I send it an Iterable
? They can iterate it once to figure out everything in the list and then they can iterate it again to remove things. And we already have an Iterable on the call site so I don't need to build any more objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
* Implementations <strong>may</strong> reorder the list but they should | ||
* be careful in doing so as the original order is important (see above). | ||
* An implementation that sorts list consistently will consistently send | ||
* requests to s single node, overloading it. So implementations that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/s single node/a single node
} | ||
|
||
/** | ||
* Returns a new {@link RestClientBuilder} to help with {@link RestClient} creation. | ||
* Creates a new builder instance and sets the hosts that the client will send requests to. | ||
* <p> | ||
* Prefer this to {@link #builder(Node...)} if you have metadata up front about the nodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer this to {@link #builder(HttpHost...)} ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
if (node == null) { | ||
throw new IllegalArgumentException("node cannot be null"); | ||
} | ||
authCache.put(node.getHost(), new BasicScheme()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were previously deduplicating hosts, isn't that needed anymore?
assertThat(deadHostState.shallBeRetried(), is(true)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test should be restored
}; | ||
|
||
long nanoTime(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose these changes are gone due to a bad merge. I think this was a good improvement and I don't see a replacement for it and the corresponding tests in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of. In my old PR DeadHostState
didn't have a shallBeRetried
because it looks at the getDeadUntilNanos
directly so it can sort all the dead nodes. That was part of the thing that allowed us to get rid of the looping. I pulled on the string from there and got here. I agree about the missing test - I need to rebuild it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to use the fact that DeadHostState is now Comparable and the shallBeRetried method was convenient too I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the same here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see what you are getting at. We can keep shallbeRetried
if we keep the DeadHostState
and sort on it. That'd be fine.
if (false == deadNodes.isEmpty()) { | ||
Collections.sort(deadNodes, new Comparator<DeadNodeAndRevival>() { | ||
@Override | ||
public int compare(DeadNodeAndRevival lhs, DeadNodeAndRevival rhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeadHostState is already Comparable
, can we rely on that here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been wondering about combining the Node
into DeadHostState
which I think would make this simpler. I'm kind of weary to do it as part of this already large change though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't want to introduce more changes in this PR, but I would like to restore some of the previous stuff around comparable that was recently introduced.
livingNodes.add(node); | ||
continue; | ||
} | ||
deadNodes.add(new DeadNodeAndRevival(node, nanosUntilRevival)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call the class DeadNodeState ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to clash with DeadHostState
. They sort of do different things too.
* the selector is ok with any over the living nodes then use | ||
* them for the request. | ||
*/ | ||
// TODO this is going to send more requests to nodes right *after* a node that the selector removes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point! Can we have a test for this? And think about how to fix it next? :)
On one hand it is very problematic to have a different node selector per request (I am even wondering now why we are exposing that, I forgot :) ), as if the selector is not consistent, we can't be either when round-robining?
On the other hand, now we mess up even in case the node selector does the same thing over and over, which is bad.
Maybe we should simply take out the ordering aspect here then and perform the rotate after calling the node selector, and make it explicit that the node selector should only select and never rely on ordering? Maybe have a set instead that makes it even more explicit?
Another unrelated problem is that we have an AtomicInteger
counter... that's going to overflow at some point. Not sure but maybe while we revise this rotate problem we also have a chance to fix the counter limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to revisit this in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one makes me very nervous. As we discussed yesterday, couldn't we just move the rotate after the select and be done with it? Or do you foresee more changes needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we can indeed put the rotate after the select now that we've given up on the order meaning anything to folks that implement NodeSelector
.
&& Objects.equals(boundHosts, other.boundHosts) | ||
&& Objects.equals(version, other.version) | ||
&& Objects.equals(name, other.name) | ||
&& Objects.equals(roles, other.roles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may sound controversial but in my mind the answer to the "when are two nodes equal?" question is "when they have the same host and port".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think things that want to compare nodes based on the host and port should do lhs.getHost().equals(rhs.getHost())
. I think most of the time they really do want to compare by host like you say, but I think we can be explicit those time. Mostly it just really confuses me when things ignore parts of themselves in equality checks....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we then drop these methods in this case, I think it confuses me to have equals working this way if most of the times we want to compare the hosts :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I just suggested that we re-implement this. Do what you prefer, I am fine with both at this point given that depending on the day I suggest to have equals or move it to tests :)
@@ -367,4 +373,76 @@ private String errorMessage(ExecutableSection executableSection, Throwable t) { | |||
protected boolean randomizeContentType() { | |||
return true; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust the indentation?
* thread safe but it doesn't have to be for our tests. | ||
* </ul> | ||
*/ | ||
private void sniffHostMetadata(RestClient client) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about removing the need for this and for boundHosts (I think!) by enabling sniffing in our yaml tests? wouldn't that be a good way to test it too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that'd be safe, but it feels like it is a larger change than this. I'll have to think about it some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
larger change in terms of lines of code? Or it just does not belong here? My idea was to reduce the size of the PR to just enabling sniffing in the yaml test suite. But maybe I am not seeing where sniffing may cause problems.
I've merged master to this and I'm going to apply feedback now and try write some docs. At least that is my plan. |
Stll a few comments left to get through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great thanks @nik9000 for being patient with me :) I left a few nits and I think the TODO in the docs may need to be addressed, LGTM besides that
if (false == selectedDeadNodes.isEmpty()) { | ||
return singletonList(selectedDeadNodes.get(0)); | ||
return singletonList(Collections.min(selectedDeadNodes).node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe silly question: how does min
help compared to sorting? Doesn't min iterate over all the elements?
if (timeSupplier != other.timeSupplier) { | ||
throw new IllegalArgumentException("can't compare DeadHostStates with different clocks [" | ||
+ timeSupplier + " != " + other.timeSupplier + "]"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
long timeoutNanos = (long)Math.min(MIN_CONNECTION_TIMEOUT_NANOS * 2 * Math.pow(2, previousDeadHostState.failedAttempts * 0.5 - 1), | ||
MAX_CONNECTION_TIMEOUT_NANOS); | ||
this.deadUntilNanos = timeSupplier.nanoTime() + timeoutNanos; | ||
this.deadUntilNanos = previousDeadHostState.timeSupplier.nanoTime() + timeoutNanos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
/** | ||
* Selector that matches any node that has metadata and doesn't | ||
* have the {@code master} role OR it has the data {@code data} | ||
* role. It does not reorder the nodes sent to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reordering bit can be removed given that it can't reorder anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Map<HttpHost, Node> nodesByHost = new LinkedHashMap<>(); | ||
for (Node node : nodes) { | ||
Objects.requireNonNull(node, "node cannot be null"); | ||
// TODO should we throw an IAE if this happens? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify the comment so it is clear that it's about nodes with same host?
* Contains a reference to a blacklisted node and the time until it is | ||
* revived. We use this so we can do a single pass over the blacklist. | ||
*/ | ||
private static class DeadNodeAndDeadness implements Comparable<DeadNodeAndDeadness> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is kind of a funny name to me, I would be ok with DeadNode
, not a biggie though, it's a private class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will rename.
for (int i = 0; i < numHttpServers; i++) { | ||
HttpServer httpServer = createHttpServer(); | ||
httpServers[i] = httpServer; | ||
httpHosts[i] = new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort()); | ||
} | ||
restClient = buildRestClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation looks off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. Not sure why I did that. Fixed.
@@ -188,6 +190,14 @@ public void onFailure(Exception exception) { | |||
request.setOptions(options); | |||
//end::rest-client-response-consumer | |||
} | |||
{ | |||
//tag::rest-client-node-selector | |||
// TODO link me to docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is left to be done?
assertEquals(expectedNode.getBoundHosts(), actualNode.getBoundHosts()); | ||
assertEquals(expectedNode.getName(), actualNode.getName()); | ||
assertEquals(expectedNode.getVersion(), actualNode.getVersion()); | ||
assertEquals(expectedNode.getRoles(), actualNode.getRoles()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you prefer moving this back to Node I am fine with it, no biggie.
ElasticsearchNodesSniffer.Scheme.valueOf(getProtocol().toUpperCase(Locale.ROOT)); | ||
ElasticsearchNodesSniffer sniffer = new ElasticsearchNodesSniffer( | ||
adminClient(), ElasticsearchNodesSniffer.DEFAULT_SNIFF_REQUEST_TIMEOUT, scheme); | ||
return sniffer.sniff(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am quite surprised that all of the yaml tests are ok with this, given that we use potentially more nodes than the configured ones. Good though, and tests that need something different will have to configure a selector. I like it. Maybe this should be documented somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Let me find a spot.
<3> Customize the response consumer. | ||
|
||
`addHeader` is for headers that are required for authorization or to work with | ||
a proxy that in front of Elasticsearch. There is no need to set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some missing word here? that is?
`HttpEntity` attached to the request. | ||
|
||
You can set the `NodeSelector` which controls which nodes will receive | ||
requests. `NodeSelector.NOT_MASTER_ONLY` is a good choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may need to become the default at some point ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.0?
I'm working through issues backporting this with tribe. |
In 6.x we need to be able to select which node receives a request by node name. This implements that. Relates to elastic#30523
Add a `NodeSelector` so that users can filter the nodes that receive requests based on node attributes. I believe we'll need this to backport elastic#30523 and we want it anyway. I also added a bash script to help with rebuilding the sniffer parsing test documents.
* master: Remove RestGetAllAliasesAction (#31308) Temporary fix for broken build Reenable Checkstyle's unused import rule (#31270) Remove remaining unused imports before merging #31270 Fix non-REST doc snippet [DOC] Extend SQL docs Immediately flush channel after writing to buffer (#31301) [DOCS] Shortens ML API intros Use quotes in the call invocation (#31249) move security ingest processors to a sub ingest directory (#31306) Add 5.6.11 version constant. Fix version detection. SQL: Whitelist SQL utility class for better scripting (#30681) [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299) CCS: don't proxy requests for already connected node (#31273) Mute ScriptedMetricAggregatorTests testSelfReferencingAggStateAfterMap [test] opensuse packaging turn up debug logging Add unreleased version 6.3.1 Removes experimental tag from scripted_metric aggregation (#31298) [Rollup] Metric config parser must use builder so validation runs (#31159) [ML] Check licence when datafeeds use cross cluster search (#31247) Add notion of internal index settings (#31286) Test: Remove broken yml test feature (#31255) REST hl client: cluster health to default to cluster level (#31268) [ML] Update test thresholds to account for changes to memory control (#31289) Log warnings when cluster state publication failed to some nodes (#31233) Fix AntFixture waiting condition (#31272) Ignore numeric shard count if waiting for ALL (#31265) [ML] Implement new rules design (#31110) index_prefixes back-compat should test 6.3 (#30951) Core: Remove plain execute method on TransportAction (#30998) Update checkstyle to 8.10.1 (#31269) Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202) Modify pipelining handlers to require full requests (#31280) Revert upgrade to Netty 4.1.25.Final (#31282) Use armored input stream for reading public key (#31229) Fix Netty 4 Server Transport tests. Again. REST hl client: adjust wait_for_active_shards param in cluster health (#31266) REST high-level Client: remove deprecated API methods (#31200) [DOCS] Mark SQL feature as experimental [DOCS] Updates machine learning custom URL screenshots (#31222) Fix naming conventions check for XPackTestCase Fix security Netty 4 transport tests Fix race in clear scroll (#31259) [DOCS] Clarify audit index settings when remote indexing (#30923) Delete typos in SAML docs (#31199) REST high-level client: add Cluster Health API (#29331) [ML][TEST] Mute tests using rules (#31204) Support RequestedAuthnContext (#31238) SyncedFlushResponse to implement ToXContentObject (#31155) Add Get Aliases API to the high-level REST client (#28799) Remove some line length supressions (#31209) Validate xContentType in PutWatchRequest. (#31088) [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024) Suppress extras FS on caching directory tests Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)" Revert "Fix snippets in upgrade docs" Fix snippets in upgrade docs [DOCS] Added 6.3 info & updated the upgrade table. (#30940) LLClient: Support host selection (#30523) Upgrade to Netty 4.1.25.Final (#31232) Enable custom credentials for core REST tests (#31235) Move ESIndexLevelReplicationTestCase to test framework (#31243) Encapsulate Translog in Engine (#31220) HLRest: Add get index templates API (#31161) Remove all unused imports and fix CRLF (#31207) [Tests] Fix self-referencing tests [TEST] Fix testRecoveryAfterPrimaryPromotion [Docs] Remove mention pattern files in Grok processor (#31170) Use stronger write-once semantics for Azure repository (#30437) Don't swallow exceptions on replication (#31179) Limit the number of concurrent requests per node (#31206) Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions (#31044) Move java version checker back to its own jar (#30708) [test] add fix for rare virtualbox error (#31212)
Add a `NodeSelector` so that users can filter the nodes that receive requests based on node attributes. I believe we'll need this to backport #30523 and we want it anyway. I also added a bash script to help with rebuilding the sniffer parsing test documents.
Allows users of the Low Level REST client to specify which hosts a request should be run on. They implement the `NodeSelector` interface or reuse a built in selector like `NOT_MASTER_ONLY` to chose which nodes are valid. Using it looks like: ``` Request request = new Request("POST", "/foo/_search"); RequestOptions options = request.getOptions().toBuilder(); options.setNodeSelector(NodeSelector.NOT_MASTER_ONLY); request.setOptions(options); ... ``` This introduces a new `Node` object which contains a `HttpHost` and the metadata about the host. At this point that metadata is just `version` and `roles` but I plan to add node attributes in a followup. The canonical way to **get** this metadata is to use the `Sniffer` to pull the information from the Elasticsearch cluster. I've marked this as "breaking-java" because it breaks custom implementations of `HostsSniffer` by renaming the interface to `NodesSniffer` and by changing it from returning a `List<HttpHost>` to a `List<Node>`. It *shouldn't* break anyone else though. Because we expect to find it useful, this also implements `host_selector` support to `do` statements in the yaml tests. Using it looks a little like: ``` --- "example test": - skip: features: host_selector - do: host_selector: version: " - 7.0.0" # same syntax as skip apiname: something: true ``` The `do` section parses the `version` string into a host selector that uses the same version comparison logic as the `skip` section. When the `do` section is executed it passed the off to the `RestClient`, using the `ElasticsearchHostsSniffer` to sniff the required metadata. The idea is to use this in mixed version tests to target a specific version of Elasticsearch so we can be sure about the deprecation logging though we don't currently have any examples that need it. We do, however, have at least one open pull request that requires something like this to properly test it. Closes #21888
Add a `NodeSelector` so that users can filter the nodes that receive requests based on node attributes. I believe we'll need this to backport #30523 and we want it anyway. I also added a bash script to help with rebuilding the sniffer parsing test documents.
Backport (finally) complete. |
* 6.x: Add get stored script and delete stored script to high level REST API Increasing skip version for failing test on 6.x Skip get_alias tests for 5.x (#31397) Fix defaults in GeoShapeFieldMapper output (#31302) Test: better error message on failure Mute DefaultShardsIT#testDefaultShards test Fix reference to XContentBuilder.string() (#31337) [DOCS] Adds monitoring breaking change (#31369) [DOCS] Adds security breaking change (#31375) [DOCS] Backports breaking change (#31373) RestAPI: Reject forcemerge requests with a body (#30792) Docs: Use the default distribution to test docs (#31251) Use system context for cluster state update tasks (#31241) [DOCS] Adds testing for security APIs (#31345) [DOCS] Removes ML item from release highlights [DOCS] Removes breaking change (#31376) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) Expose lucene's RemoveDuplicatesTokenFilter (#31275) [Test] Fix :example-plugins:rest-handler on Windows Delete typos in SAML docs (#31199) Ensure we don't use a remote profile if cluster name matches (#31331) Test: Skip alias tests that failed all weekend [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC Add ingest-attachment support for per document `indexed_chars` limit (#31352) SQL: Fix rest endpoint names in node stats (#31371) [DOCS] Fixes small issue in release notes Support for remote path in reindex api Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Remove trial status info from start trial doc (#31365) [DOCS] Added links in breaking changes pages [DOCS] Adds links to release notes and highlights Docs: Document changes in rest client QA: Fix tribe tests to use node selector REST Client: NodeSelector for node attributes (#31296) LLClient: Fix assertion on windows LLClient: Support host selection (#30523) Add QA project and fixture based test for discovery-ec2 plugin (#31107) [ML] Hold ML filter items in sorted set (#31338) [ML] Add description to ML filters (#31330)
Allows users of the Low Level REST client to specify which hosts a
request should be run on. They implement the
NodeSelector
interfaceor reuse a built in selector like
NOT_MASTER_ONLY
to chose which nodesare valid. Using it looks like:
This introduces a new
Node
object which contains aHttpHost
and themetadata about the host. At this point that metadata is just
version
and
roles
but I plan to add node attributes in a followup. Thecanonical way to get this metadata is to use the
Sniffer
to pullthe information from the Elasticsearch cluster.
I've marked this as "breaking-java" because it breaks custom
implementations of
HostsSniffer
by renaming the interface toNodesSniffer
and by changing it from returning aList<HttpHost>
to aList<Node>
. It shouldn't break anyone else though.Because we expect to find it useful, this also implements
host_selector
support to
do
statements in the yaml tests. Using it looks a littlelike:
The
do
section parses theversion
string into a host selector thatuses the same version comparison logic as the
skip
section. When thedo
section is executed it passed the off to theRestClient
, usingthe
ElasticsearchHostsSniffer
to sniff the required metadata.The idea is to use this in mixed version tests to target a specific
version of Elasticsearch so we can be sure about the deprecation
logging though we don't currently have any examples that need it. We do,
however, have at least one open pull request that requires something
like this to properly test it.
Closes #21888