Low Level REST Client: Support host selection #29211

nik9000 · 2018-03-22T20:44:31Z

Adds support users of the Low Level REST Client to implement the new
HostSelector interface to select the hosts to which to send requests.
For example, this HostSelector selects only hosts which cannot be
master nodes:

    /**
     * Selector that matches any node that has metadata and doesn't
     * have the {@code master} role. These nodes cannot become a "master"
     * node for the Elasticsearch cluster.
     */
    HostSelector NOT_MASTER = new HostSelector() {
        @Override
        public boolean select(HttpHost host, HostMetadata meta) {
            return meta != null && false == meta.roles().master();
        }

        @Override
        public String toString() {
            return "NOT_MASTER";
        }
    };

Consumers on Java 8 should be able to use lambdas like this:

HostSelector notIngest = (host, meta) -> meta != null
        && false == meta.roles().ingest();

At this point the HostMetadata contains version and roles and is
canonically populated by ElasticsearchHostsSniffer which parses this
information from the /_nodes API while it is sniffing for hosts.
Configuring the Sniffer will automatically set this metadata.

Instead of creating yet another variant of performRequest and
performRequestAsync this adds a withHostSelector method to
RestClient that creates a "stateless view" into the RestClient. It is
"stateless" in that it backs of its operations to the RestClient that
created it so it doesn't need any sort of try-with-resources tracking,
nor does it implement Closeable.

This same pattern could be used in the future to reduce the number of
variant of performRequest and performRequestAsync by moving some of
the less commonly used parameters with other withXXX style methods.

I've done no work to get the HighLevelRestClient working with these
"stateless views" so that will have to wait for a followup.

I've marked this as "breaking-java" because it breaks custom
implementations of HostsSniffer. It also might cause trouble for folks
that mock RestClient. The new interface RestClientActions is much
more amenable to mocking.

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 (kind of, it isn't in the high level client, but we'll do that in a followup)

Adds selector support to tests, plumbs selectors some more.

Adds `host_selector` support to `do` statements in the yaml tests. Using it looks a little like: ``` --- "some example": - skip: features: host_selector - do: host_selector: version: " - 7.0.0" # same syntax as skip apiname: something: true ``` This is implemented by using `ElasticsearchHostsSniffer` in the test framework to load sniff metadata about all hosts and stick it on a `RestClient`. The `do` section contains a `HostSelector` that it hands off to the `RestClient`. The idea is to use this in mixed version tests to target a specific version of Elasticsearch.

elasticmachine · 2018-03-22T20:44:33Z

Pinging @elastic/es-core-infra

nik9000 · 2018-03-22T20:45:34Z

I've marked this as "breaking-java" because it breaks custom
implementations of HostsSniffer. It also might cause trouble for folks
that mock RestClient. The new interface RestClientActions is much
more amenable to mocking.

I'm not sure how strong our commitment to keeping the Low Level REST Client backwards compatible across releases is. Personally I feel like it is new enough that some bumps along the road for things like this are normal, but I understand that they are far from pleasant.

nik9000 · 2018-03-22T20:47:13Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

    private final CloseableHttpAsyncClient client;
    // We don't rely on default headers supported by HttpAsyncClient as those cannot be replaced.
    // These are package private for tests.
    final List<Header> defaultHeaders;
    private final long maxRetryTimeoutMillis;
    private final String pathPrefix;
    private final AtomicInteger lastHostIndex = new AtomicInteger(0);
-    private volatile HostTuple<Set<HttpHost>> hostTuple;


I moved this so the mutable stuff was below the immutable stuff.

I feel like it makes it easier to read.

nik9000 · 2018-03-22T20:48:03Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

        this.blacklist.clear();
    }

    /**
-     * Sends a request to the Elasticsearch cluster that the client points to and waits for the corresponding response


All of these javadocs are now on the RestClientActions interface.

nik9000 · 2018-03-22T20:48:46Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-     * @throws ClientProtocolException in case of an http protocol error
-     * @throws ResponseException in case Elasticsearch responded with a status code that indicated an error
-     */
+    // TODO this exists entirely to so we don't have to change much in the high level rest client tests. We'll remove in a followup.


I want to remove this in the followup that adapts the high level REST client to this change.

nik9000 · 2018-03-22T20:57:35Z

but I understand that they are far from pleasant.

And I'm happy to be convinced to do more with regards to backwards compatibility.

nik9000 · 2018-03-22T22:36:03Z

client/rest/src/main/java/org/elasticsearch/client/DeadHostState.java

@@ -53,6 +53,11 @@ private DeadHostState() {
        this.failedAttempts = previousDeadHostState.failedAttempts + 1;
    }

+    DeadHostState(int failedAttempts, long deadUntilNanos) {


I'm not sure about this - it helps with testing but I expect it conflicts with something @javanna is doing now.

this is not a big deal, I will split my work in smaller PRs. the one around max retry timeout will need rewriting at this point but that's ok.

nik9000 · 2018-03-22T22:37:08Z

Because we expect to find it useful, this also implements host_selector
support to do statements in the yaml tests.

Specifically, I think this'll be useful for #21510.

nik9000 · 2018-03-23T14:04:29Z

I need to push docs for this before merging but I expect we'll spend a while talking about this before merging.

javanna

I went through this and I like it. I left some comments but this is going in the right direction. Thanks @nik9000 !

javanna · 2018-03-23T10:29:16Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+        /**
+         * The node runs ingest pipelines.
+         */
+        public boolean ingest() {


nit: maybe use the is prefix?

has prefix instead ok?

I don't mind. maybe also call master masterEligible instead just to prevent misunderstandings?

javanna · 2018-03-23T10:29:28Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+    }
+
+    /**
+     * Role information about and Elasticsearch process.


javanna · 2018-03-23T10:30:38Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+    /**
+     * Version of Elasticsearch that the host is running.
+     */
+    private final String version;


wondering if strings are enough for version comparisons etc. but surely using our own Version class is not a good idea here.

I think the high level rest client will have access to our Version class and can compare. That is kind of what I do in the yaml tests further down. I don't think it is worth building our own Version class, yeah.

I think a string is the right thing, at least for now. It all depends on how much we can break this in the future as things get better.

unfortunately the Version class is the one that currently brings lucene in :) but yea I tend to agree that for now we are good. I was contemplating making it a number for better comparisons, but that may prove even more confusing to users?

Yeah, I think it'd be more confusing....

javanna · 2018-03-23T10:31:00Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+    public static final HostMetadataResolver EMPTY_RESOLVER = new HostMetadataResolver() {
+        @Override
+        public HostMetadata resolveMetadata(HttpHost host) {
+            return null;


shall we rather have an EMPTY placeholder here and avoid null values? Or use Optional ?

Optional is a Java 8 thing.

I'm not sure what the right semantics for EMPTY would be. Passing null requires the implementer to think about what they want to do if they don't have any metadata which I kind of like. I think Optional would be more clear, but we don't have it.

oh I thought it was introduced much earlier. Maybe in this case null is ok here

javanna · 2018-03-23T10:38:08Z

client/rest/src/main/java/org/elasticsearch/client/HostSelector.java

+     * have the {@code master} role. These nodes cannot become a "master"
+     * node for the Elasticsearch cluster.
+     */
+    HostSelector NOT_MASTER = new HostSelector() {


maybe we should instead have one to skip master only nodes? master nodes are not to be avoided unless you have dedicated masters and dedicated data nodes.

Makes sense to me. I'll replace this with that. I wanted an example that folks would find useful in real code too. And NOT_MASTER_ONLY would be better. It should only filter out nodes that are master nodes but are not data nodes, right? What about ingest? I think it should ignore that role to be honest.

I would filter out nodes that are only master, so if they happen to be master eligible and also ingest, I would keep them in the loop. But this is an edge case, the important bit is definitely to take out what's master but not data like you said.

javanna · 2018-03-23T15:17:32Z

client/rest/src/main/java/org/elasticsearch/client/HostSelector.java

+        @Override
+        public String toString() {
+            return "ANY";
+        }


do we need toString for the Host selector impls?

javanna · 2018-03-23T15:19:49Z

client/rest/src/main/java/org/elasticsearch/client/RestClientView.java

+/**
+ * Stateless view into a {@link RestClient} with customized parameters.
+ */
+class RestClientView extends AbstractRestClientActions {


I would love to find a better name for this. I don't think view describes it well, but I haven't come up with anything better yet :)

I'm not really happy with any of the names.

maybe RestClientWrapper but its not much better than RestClientView :)

Great to see an immutable view added! I wanted to ask for one ;)

We plan to allow Hibernate Search users to get a reference to the RestClient so that people can use the occasional "low-level" operation but I wasn't happy to expose some of the mutators.

https://hibernate.atlassian.net/browse/HSEARCH-3125?focusedCommentId=101943&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-101943

I'd talked about this with @javanna a while ago and we're kind of abandoning this approach because we think it is too confusing. We won't have a view, I think.

Maybe the fact we have a good use case will help make a case for it :)

BTW I would just need the interface: I believe re-defining the interface just to remove some methods in Hibernate is not ideal as it would imply a high coupling, while I'd like to expose your interface so that people using the latest Elasticsearch would have any new features immediately, w/o us having to update our facade first. (Normally we'd have a facade to better maintain the API but this is for advanced users, which will prefer to not have anything in the way)

javanna · 2018-03-23T15:20:42Z

client/rest/src/test/java/org/elasticsearch/client/documentation/RestClientDocumentation.java


+    public void testHostSelector() throws IOException {


is this snippet used although it has no tags for inclusion in the docs?

I need to rework it and add it to the docs, I think.

javanna · 2018-03-23T15:24:08Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+     * <li>It only runs once
+     * </ul>
+     */
+    protected void sniffHostMetadata(RestClient client) throws IOException {


why is the method here and not where it's used?

I'll move it. I started out thinking it was something that'd have to be done at construction time but it turned out not to be the case. Then I figured it was a thing that other ESRestTestCase subclasses might use. But at this point it doens't make a lot of sense. So I'll move it.

javanna · 2018-03-23T15:24:32Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+        ElasticsearchHostsSniffer sniffer = new ElasticsearchHostsSniffer(
+            adminClient, ElasticsearchHostsSniffer.DEFAULT_SNIFF_REQUEST_TIMEOUT, scheme);
+        Map<HttpHost, HostMetadata> meta = sniffer.sniffHosts().hostMetadata();
+        client.setHosts(clusterHosts, meta::get);


this is an interesting use of the sniffer classes! hopefully the configured hosts match the ones found in the cluster state :)

They, uh, have so far!

Now that I think about it I think I should try and be a little more careful here and assert that all the hosts come back. The test would fail in a weird way if I don't.

nik9000 · 2018-03-23T15:52:14Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-                    filteredHosts.remove(entry.getKey());
+            final HostTuple<Set<HttpHost>> hostTuple = this.hostTuple;
+            result = nextHostsOneTime(hostTuple, blacklist, lastHostIndex, System.nanoTime(), hostSelector);
+            if (result.hosts == null) {


You mean default result.hosts to emptyList and then check for .isEmpty instead. Hmmmm. I wonder if that'd be easier to read to be honest? I feel like if I was reading it I might think "isEmpty will catch more than just the default case." Maybe! I'll have a think on it.

nik9000 · 2018-03-23T15:55:51Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-                    });
-                    HttpHost deadHost = sortedHosts.get(0).getKey();
+            attempts++;
+        } while (attempts < MAX_NEXT_HOSTS_ATTEMPTS);


Hmmm. We can fail immediately here if the selector rejected all of the dead hosts, but we can't know that we didn't get the concurrent modification of the hosts that shifted a host that the selector wouldn't have rejected out of the dead hosts list. Basically I'm saying I'm not sure we can be sure which of the two things is true because the second reason might be made worse by the first reason.

If dead hosts were stored in another way we could figure out if they changed and retry if they did, but they aren't and I think maybe that should be another PR if we want to change it.

nik9000 · 2018-03-23T16:01:00Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-                    HttpHost deadHost = sortedHosts.get(0).getKey();
+            attempts++;
+        } while (attempts < MAX_NEXT_HOSTS_ATTEMPTS);
+        throw new IOException("No hosts available for request. Last failure was " + result.describeFailure());


Yeah, it doesn't give any context because sometimes we're reviving a dead node and sometimes we're picking from a list of living ones.

It feels right to me for HostSelectors to make very simple choices so it is easier to reason about what they do.

I understand wanting to make the "what if we reject all the hosts" behavior pluggable, but I think it is pretty complicated because you don't know if you rejected all of the hosts or just all of the living hosts.

What if we make our own subclass of IOException and folks can catch it and retry without the selector if they really want to? That feels like an exceptional case so I don't feel too bad about it. I'm not sure though.

nik9000 · 2018-03-23T16:01:38Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        throw new IOException("No hosts available for request. Last failure was " + result.describeFailure());
+    }
+
+    static class NextHostsResult {


That is an interesting thing! That'd be nice!

nik9000 · 2018-03-23T16:02:14Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+    static class NextHostsResult {
+        /**
+         * Number of hosts filtered from the list because they are
+         * dead.


I suppose that is more correct. Less dramatic, but more correct. I'll change it if we keep the class.

nik9000 · 2018-03-23T16:23:16Z

client/rest/src/test/java/org/elasticsearch/client/documentation/RestClientDocumentation.java


+    public void testHostSelector() throws IOException {


I need to rework it and add it to the docs, I think.

nik9000 · 2018-03-23T16:24:19Z

client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchHostsSniffer.java

-            return null;
+        } else {
+            logger.trace("adding node [" + nodeId + "]");
+            assert sawRoles : "didn't see roles for [" + nodeId + "]";


Damn. I meant to add a test that parses the roles from an old call. I'll do that and fix this up.

nik9000 · 2018-03-23T16:26:43Z

client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchHostsSniffer.java

+            hosts.add(publishedHost);
+            HostMetadata meta = new HostMetadata(version, new Roles(master, data, ingest));
+            for (HttpHost bound: boundHosts) {
+                hostMetadata.put(bound, meta);


Because we don't know how the client is configured. In particular, at least on my machine, our REST tests are configured against an ipv4 address but the published host is an ipv6 address.

You don't need both if you are using the sniffer to sniff the hosts, but you need it if you are using the sniffer to sniff metadata for some hosts you already have.

nik9000 · 2018-03-23T16:27:34Z

client/sniffer/src/main/java/org/elasticsearch/client/sniff/SnifferResult.java

+/**
+ * Result of sniffing hosts.
+ */
+public final class SnifferResult {


Yeah, if I can make the Node concept make sense I'll do that!

nik9000 · 2018-03-23T16:28:56Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+        ElasticsearchHostsSniffer sniffer = new ElasticsearchHostsSniffer(
+            adminClient, ElasticsearchHostsSniffer.DEFAULT_SNIFF_REQUEST_TIMEOUT, scheme);
+        Map<HttpHost, HostMetadata> meta = sniffer.sniffHosts().hostMetadata();
+        client.setHosts(clusterHosts, meta::get);


They, uh, have so far!

Now that I think about it I think I should try and be a little more careful here and assert that all the hosts come back. The test would fail in a weird way if I don't.

nik9000

@javanna I've reworked a whole bunch of things:

Node instead of HostMetadata
NodeSelector (instead of HostSelector) takes and returns a List<Node>. The lists control the ordering.
I fixed the tests in all the ways you asked.

So I think this is worth another look. I know of a few things it needs before it is done though:

Docs
Better names for things.
Tests that run against real /_nodes API responses.
I need to revert my NOCOMMIT.

nik9000 · 2018-03-23T16:51:43Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+    /**
+     * Version of Elasticsearch that the host is running.
+     */
+    private final String version;


Yeah, I think it'd be more confusing....

nik9000 · 2018-03-23T16:52:14Z

client/rest/src/main/java/org/elasticsearch/client/HostMetadata.java

+    /**
+     * Roles that the Elasticsearch process on the host has.
+     */
+    private final Roles roles;


Yeah. I was trying to keep this PR from getting huge. I didn't accomplish that, but I tried.

nik9000 · 2018-03-26T22:01:59Z

client/rest/src/main/java/org/elasticsearch/client/RestClientView.java

+                return inner.select(host, meta) && hostSelector.select(host, meta);
+            }
+        };
+        return new RestClientView(delegate, combo);


I think you might do this if you have an application that uses a RestClientActions instance for "normal" operations that has NodeSelector.NOT_MASTER_ONLY and sometimes you want to send, say, nodes on a certain rack. If you do that then you'd call withNodeSelector really early to add the NOT_MASTER_ONLY selector and then call withNodeSelector many times, much later, to add the rack awareness. But only for those requests that care about it.

Or maybe you want to combine two selectors but don't want to worry about implementing AND logic.

nik9000 · 2018-03-26T22:04:02Z

client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java

        for (int i = 0; i < numHttpServers; i++) {
            HttpServer httpServer = createHttpServer();
            httpServers[i] = httpServer;
            httpHosts[i] = new HttpHost(httpServer.getAddress().getHostString(), httpServer.getAddress().getPort());
        }
+        httpServers[0].createContext(pathPrefix + "/firstOnly", new ResponseHandler(200));


I suppose it is good enough. I wanted an extra layer but I don't really need it to be honest.

nik9000 · 2018-03-27T14:30:49Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+     * <li>It only runs once
+     * </ul>
+     */
+    protected void sniffHostMetadata(RestClient client) throws IOException {


I'll move it. I started out thinking it was something that'd have to be done at construction time but it turned out not to be the case. Then I figured it was a thing that other ESRestTestCase subclasses might use. But at this point it doens't make a lot of sense. So I'll move it.

nik9000 · 2018-03-27T19:21:12Z

client/rest/src/main/java/org/elasticsearch/client/AbstractRestClientActions.java

+ * Abstract implementation of {@link RestClientActions} shared by
+ * {@link RestClient} and {@link RestClientView}.
+ */
+abstract class AbstractRestClientActions implements RestClientActions {


I use this so I only have to implement all the performRequest flavors. Without it I'd basically copy and paste the guts of it into both RestClientView and RestClient. Or I could declare 8 more methods in RestClient that take the NodeSelector and then delegate directly to them. I don't think either of those is particularly clean though.

I get wanting to return the abstract class and node having an interface at all, but I feel like the interface makes mocking simpler and it will allow us to change more things later without them being breaking changes.

javanna

@nik9000 I did another pass, I have yet to look at the sniffing changes and tests, but I can only do it later today or tomorrow, so I am sending out now the comments that I have till now.

javanna · 2018-03-28T13:09:06Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+    /**
+     * Make a copy of this {@link Node} but replacing its
+     * {@link #getHost() host}. Use this when the sniffing implementation
+     * returns returns a {@link #getHost() host} that is not useful to the


double "returns"

javanna · 2018-03-28T13:10:28Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+        /**
+         * The node runs ingest pipelines.
+         */
+        public boolean hasIngest() {


man now that I see it I do prefer is over has :)

javanna · 2018-03-28T13:12:54Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+     *      that the {@link RestClient} would prefer to use them
+     * @return a subset of the provided list of {@linkplain Node}s that the
+     *      selector approves of, in the order that the selector would prefer
+     *      to use them.


can we clarify in the docs when this gets called so it is clear that we call it for each single request, and the list holds all the nodes that will be tried for that request, in case there are failures that need to be retried?

javanna · 2018-03-28T13:19:44Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

    }

    /**
     * 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.
+     * Creates a new builder instance and sets the nodes that the client will send requests to.
+     * @see Node#Node(HttpHost)


maybe the other variant that accepts Nodes should have the @see Node#Node(HttpHost) but this one shouldn't?

++. Was a copy and paste mistake.

javanna · 2018-03-28T13:22:30Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-        performRequestAsyncNoCatch(method, endpoint, params, entity, httpAsyncResponseConsumerFactory,
-            listener, headers);
-        return listener.get();
+    public Node[] getNodes() { // TODO is it ok to expose this? It feels excessive but we do use it in tests.


make it package private?

I see that we need it from another package, I think it's ok.

It is used in sniffHostMetadata so we only sniff one time. That is off in the test framework.

javanna · 2018-03-28T14:43:58Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+                return singletonList(selectedDeadNodes.get(0).getHost());
+            }
+        }
+        throw new IOException("NodeSelector [" + nodeSelector + "] rejcted all nodes, "


if you throw above, maybe throw inside, only in case the selector returns empty here too, that might make the code a bit more readable.

javanna · 2018-03-28T14:47:32Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+                    subset.add(node);
+                }
+            }
+            return subset;


Maybe we should try and make this a bit smarter? In the extreme case that there are no alive non-master only nodes around, do we want to make the client resurrect one of the dead nodes (I'd say we should rather throw exception when we return empty from here)? Maybe we should rather use a master-only node in this specific case? While I write I am actually not sure about this. Maybe instead of overloading the master eligible nodes we should just bail :) But document this behaviour clearly?

I worry that "smarter" become "hard to understand" in this case. I think it makes sense to have the built in rule be hard and fast and if folks want to be more flexible in their application they can be.

One issue is that it is kind of hard to tell if you are being called with a list of live nodes or blacklisted ones. We could pass a boolean to communicate that, but that feels pretty complicated.

you are right, this is good enough, and I agree at this point that we should try and revive a node if the selector returns empty.

javanna · 2018-03-28T14:49:03Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+     * The maximum number of attempts that {@link #nextNode(NodeSelector)} makes
+     * before giving up and failing the request.
+     */
+    private static final int MAX_NEXT_NODES_ATTEMPTS = 10;


This is no longer used, could be removed

javanna · 2018-03-28T14:56:13Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        List<Node> livingNodes = new ArrayList<>(nodeTuple.nodes.size() - blacklist.size());
+        List<DeadNodeAndRevival> deadNodes = new ArrayList<>(blacklist.size());
+        for (Node node : nodeTuple.nodes) {
+            DeadHostState deadness = blacklist.get(node.getHost());


I was hesitant in the beginning due to the need for the additional class, but this looks great, it simplifies also concurrency as we read only once per node from the blacklist, so from then on we don't even have to retry as we kind of act based on the previous view of the blacklist. Not only does it allow us to remove the max_attempt, but also to remove retries due to the concurrent changes as a whole!

javanna · 2018-03-28T15:00:07Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+            throw new IllegalArgumentException("host cannot be null");
+        }
+        this.host = host;
+        this.boundHosts = boundHosts;


I am not sure that we still need this given that users can provide metadata together with the corresponding host. Can we remove this?

We still use this in the yaml testing framework to look up metadata.

And add tests that parse the `/_nodes/http` API responses from older versions so we verify that we don't break it.

javanna

Leaving a bunch of new comments, review round completed. This looks great, comments are all minor except a couple of them, especially the one on including node attributes when sniffing metadata, sniffing metadata from 2.x and retry logic in case the selector returns no nodes.

I will look closer at docs and naming as part of the next review round. As for testing against real responses I am fine if it is unit tests that include responses from different ES versions. We may want to add proper integration tests also for the sniffer as it is not well tested but that can come as a follow-up I think.

javanna · 2018-03-28T19:45:35Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

     */
-    public Response performRequest(String method, String endpoint, Header... headers) throws IOException {
-        return performRequest(method, endpoint, Collections.<String, String>emptyMap(), null, headers);
+    public void setHosts(HttpHost... hosts) {


I think that we could remove this if we wish to. I feel like the method that everybody is using is RestClient#builder(HttpHost...) and is nice to keep also as a shortcut, while setHosts is used by the sniffer and more specialized. Yet it is no problem to keep either I think.

javanna · 2018-03-28T19:48:42Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+    static List<HttpHost> selectHosts(NodeTuple<List<Node>> nodeTuple,
+            Map<HttpHost, DeadHostState> blacklist, AtomicInteger lastNodeIndex,
+            long now, NodeSelector nodeSelector) throws IOException {
+        class DeadNodeAndRevival {


I think that once you merge master in this will hold Node and DeadHostState, you could rename it to DeadNode or DeadNodeState?

javanna · 2018-03-28T19:49:02Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        if (false == livingNodes.isEmpty()) {
+            /*
+             * Normal state: there is at least one living node. Rotate the
+             * list so subsequent requests to will see prefer the nodes in


something is wrong in this sentence :)

++. I accidentally left an extra extra word.

javanna · 2018-03-28T19:52:10Z

client/rest/src/main/java/org/elasticsearch/client/RestClientView.java

+/**
+ * Stateless view into a {@link RestClient} with customized parameters.
+ */
+class RestClientView extends AbstractRestClientActions {


maybe RestClientWrapper but its not much better than RestClientView :)

javanna · 2018-03-28T19:53:35Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+     * if we don't know what roles the node has.
+     */
+    private final Roles roles;
+


we need node attributes in here too

I kind of want to do it in a followup if that is ok with you.

ok as long as we don't forget :)

javanna · 2018-03-28T20:17:59Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+     * returns returns a {@link #getHost() host} that is not useful to the
+     * client.
+     */
+    public Node withHost(HttpHost host) {


call it replaceHost?

javanna · 2018-03-28T20:18:46Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-        performRequestAsyncNoCatch(method, endpoint, params, entity, httpAsyncResponseConsumerFactory,
-            listener, headers);
-        return listener.get();
+    public Node[] getNodes() { // TODO is it ok to expose this? It feels excessive but we do use it in tests.


I see that we need it from another package, I think it's ok.

javanna · 2018-03-28T20:19:49Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-            listener, headers);
-        return listener.get();
+    public Node[] getNodes() { // TODO is it ok to expose this? It feels excessive but we do use it in tests.
+        return nodeTuple.nodes.toArray(new Node[0]);


nodes are immutable, so there can't be changes made to the returned array which can affect the running rest client right? Can you use the known size here rather than 0?

I think I can get away with just returning the List which is indeed immutable. I'll try that.

javanna · 2018-03-28T20:22:52Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

-        }
-        this.hostTuple = new HostTuple<>(Collections.unmodifiableSet(httpHosts), authCache);
-        this.blacklist.clear();
+    public static RestClientBuilder builder(Node... nodes) {


could we clarify in the docs why one would use this over the other one and what a Node is for compared to a host?

javanna · 2018-03-28T20:29:12Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+     * first and then running the "left" selector on the results of the "right"
+     * selector.
+     */
+    class Compose implements NodeSelector {


I am pretty sure that we don't need this, especially given that the only time we use it in our tests is to compose (any , selector) . It is cool but I think it adds complexity, needs testing, while a single selector can do all it wants alone, no need to chain them.

We also use it in RestClientView.withNodeSelector. I expect we want to keep withNodeSelector on RestClientView because we'll end up with other withXXX style methods which will all return RestClientView. So folks will do client.withTime(123).withNodeSelector(bar).

We could implement RestClientView.withNodeSelector to check if the NodeSelector in the view is ANY and throw an IllegalStateException if it isn't but that feels weird.

I would keep things simple and use SetOnce to make sure that users can only set one node selector, if not set we use ANY

oh man SetOnce is a lucene thing though :x

javanna · 2018-03-28T20:39:06Z

one more thing, on the usability aspect and naming with the new RestClientActions interface, maybe @tlrx could have a look too, I am curious to hear what he thinks.

nik9000 · 2018-03-28T22:07:54Z

RestClientActions

I'm really not sure what a good name is. I want to communicate:

You can throw it away without worrying about closing it.
You can make all the performRequest calls on it but can't do things like setHosts. I mean, we could make setHosts work by pushing it back to the real RestClient but that feels weird to me.
It really exists just to fix parameters for some number of requests.
You can keep this object around for a long time if you want or you can make a new one every time without much fuss.

javanna

I did another pass, looked deeper at javadocs, found a few small things. I think it's time to merge master in.

javanna · 2018-03-29T10:19:16Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+import org.apache.http.HttpHost;
+
+/**
+ * Metadata about an {@link HttpHost} running Elasticsearch.


Can you add some more info here maybe on the type of metadata?

javanna · 2018-03-29T10:23:03Z

client/rest/src/main/java/org/elasticsearch/client/Node.java

+        }
+        /**
+         * The node runs ingest pipelines.
+         */


When we add docs to getters should we add "Returns whether" or something like that?

javanna · 2018-03-29T10:32:59Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+ */
+public interface NodeSelector {
+    /**
+     * Select the {@link Node}s to which to send requests. This may be called


I would try and be even more explicit on what this method does (feel free to make adjustments though), something like: "Receives a sorted list of nodes, in the order that the client would try them for a single request, and returns a sorted list of nodes selected out of the provided ones. Provided nodes may be alive ones, in which case returning an empty list means that the client will try and revive one dead node taken from the blacklist. When reviving nodes, this method will be called too with a list of sorted nodes as argument, where the first one is the one that the client would revive. The first node returned by the selector is the one that will be revived."

javanna · 2018-03-29T10:35:24Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+     * return a list of nodes sorted by its preference for which node is used.
+     * If it is operating on "living" nodes that it returns function as
+     * fallbacks in case of request failures. If it is operating on dead nodes
+     * then the dead node that it returns is attempted but no others.


only the first node returned is attempted but no others

javanna · 2018-03-29T10:36:43Z

client/rest/src/main/java/org/elasticsearch/client/NodeSelector.java

+     * first and then running the "left" selector on the results of the "right"
+     * selector.
+     */
+    class Compose implements NodeSelector {


oh man SetOnce is a lucene thing though :x

javanna · 2018-03-29T10:44:52Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        return new NodeTuple<>(hosts.iterator(), nodeTuple.authCache);
+    }
+
+    static List<Node> selectHosts(NodeTuple<List<Node>> nodeTuple,


maybe this could be folded back into nextNode now?

javanna · 2018-03-29T10:47:37Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+     * If no living and no dead nodes match the selector we retry a few
+     * times to handle concurrent modifications of the list of dead nodes.
+     * We never block the thread or {@link Thread#sleep} or anything like
+     * that. If the retries fail this throws a {@link IOException}.


remove this part which doesn't apply anymore

javanna · 2018-03-29T10:56:04Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        if (false == livingNodes.isEmpty()) {
+            /*
+             * Normal state: there is at least one living node. Rotate the
+             * list so subsequent requests to will prefer the nodes in a


subsequent requests to ?

javanna · 2018-03-29T10:56:17Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+             */
+            Collections.rotate(livingNodes, lastNodeIndex.getAndIncrement());
+            List<Node> selectedLivingNodes = nodeSelector.select(livingNodes);
+            if (false == selectedLivingNodes.isEmpty()) {


javanna · 2018-03-29T10:56:57Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+         * NodeSelector so it can have its say in which nodes are ok and their
+         * ordering. If the selector is ok with any of the nodes then use just
+         * the first one in the list because we only want to revive a single
+         * node.


javanna · 2018-03-29T16:19:29Z

client/rest/src/main/java/org/elasticsearch/client/RestClientActions.java

+ * Actions that can be taken on a {@link RestClient} or the stateless views
+ * returned by {@link RestClientActions#withNodeSelector withNodeSelector}.
+ */
+public interface RestClientActions {


we talked about this new way of sending requests with @nik9000 and we said that we will look into changing the existing performRequest methods to accept a Request object, so that we can trim down the different performRequest methods that we have to two: sync and async. I feel like this is going to be more familiar to most of our users and something that we should have done from day one.

nik9000 · 2018-05-10T17:27:16Z

I've spent the morning rebuilding this on top of the Request object work (#29623) and I'll open a new version of this soon.

nik9000 added 12 commits March 20, 2018 15:27

Init

d336637

Further

753fb86

Adds selector support to tests, plumbs selectors some more.

Handle bound addresses

63d9514

Fix tests

17f92fe

Tests for nextHost

3bb20cb

Revert changes to high level client

c69d24a

Revert another

74e4655

Another revert

d6c885f

Fix javadocs

00dd77c

Cleanups

1704222

More cleanup

1170803

nik9000 added review >breaking-java v7.0.0 labels Mar 22, 2018

nik9000 requested a review from javanna March 22, 2018 20:44

Cleanup

c8d3458

nik9000 commented Mar 22, 2018

View reviewed changes

nik9000 added the v6.3.0 label Mar 22, 2018

Merge branch 'master' into rest_views

03d4fb5

nik9000 commented Mar 22, 2018

View reviewed changes

Fix javadoc

5f57bf4

javanna requested changes Mar 23, 2018

View reviewed changes

nik9000 commented Mar 23, 2018

View reviewed changes

nik9000 added 7 commits March 26, 2018 17:40

Fixup

241d7ca

Fixup from comments

84befef

Explain and rework a bit

a5a47a2

Move

a27317b

Fixup test

e931d4c

Rework NodeSelector so it takes a list

398f9a3

Unused import

ff5819a

nik9000 commented Mar 27, 2018

View reviewed changes

Spelling

0704e1d

nik9000 mentioned this pull request Mar 27, 2018

Rest client: provide the ability to define a specific timeout for a request #21789

Closed

javanna reviewed Mar 28, 2018

View reviewed changes

Fix 2.x support

b8eb23f

And add tests that parse the `/_nodes/http` API responses from older versions so we verify that we don't break it.

javanna requested changes Mar 28, 2018

View reviewed changes

nik9000 added 2 commits March 28, 2018 17:26

Cleanups

3d4535b

Remove unneeded

87cef53

More cleanup

88a3b5c

javanna reviewed Mar 29, 2018

View reviewed changes

javanna mentioned this pull request Apr 19, 2018

Java client: Avoid throwing a TimeoutException after the operation wa… #28387

Closed

bleskes added v6.3.1 v6.4.0 and removed v6.3.0 v6.3.1 labels Apr 26, 2018

nik9000 closed this May 10, 2018

nik9000 removed v6.4.0 v7.0.0 labels May 10, 2018

nik9000 mentioned this pull request May 10, 2018

LLClient: Support host selection #30523

Merged

Low Level REST Client: Support host selection #29211

Low Level REST Client: Support host selection #29211

Uh oh!

Conversation

nik9000 commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 22, 2018

Uh oh!

nik9000 commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 22, 2018

Uh oh!

nik9000 commented Mar 23, 2018

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 22, 2018 •

edited

Loading