Node selector per client rather than per request by javanna · Pull Request #31471 · elastic/elasticsearch

javanna · 2018-06-20T16:38:09Z

We have made node selectors configurable per request, but all of other language clients don't allow for that. A good reason not to do so, is that having a different node selector per request breaks round-robin.
This PR makes NodeSelector configurable only at client initialization, so it has to be the same for every request. It also contains a docs improvement on this matter, important given that a single node selector can still affect round-robin.

We have made node selectors configurable per request, but all of other language clients don't allow for that. A good reason not to do so, is that having a different node selector per request breaks round-robin. This commit makes NodeSelector configurable only at client initialization. It also improves the docs on this matter, important given that a single node selector can still affect round-robin.

elasticmachine · 2018-06-20T16:38:11Z

Pinging @elastic/es-core-infra

nik9000

Man, I liked being able to customize it but I understand the trade offs here. The test case work looks right to me.

nik9000 · 2018-06-21T13:03:09Z

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

     * role.
     */
-    NodeSelector NOT_MASTER_ONLY = new NodeSelector() {
+    NodeSelector SKIP_DEDICATED_MASTERS = new NodeSelector() {


nik9000 · 2018-06-21T13:05:07Z

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

-                    assertThat(e.getMessage(), startsWith("Connection refused"));
+        try (RestClient restClient = buildRestClient(firstPositionNodeSelector())) {
+            Request request = new Request("GET", "/200");
+            RequestOptions.Builder options = request.getOptions().toBuilder();


I don't think we need RequestOptions.Builder at all here.

nik9000 · 2018-06-21T13:07:42Z

docs/java-rest/low-level/configuration.asciidoc

+no nodes in such case which would make the client forcibly revive a local node
+whenever none of the nodes from the preferred rack is available.
+
+WARNING: Node selectors that select a different set of nodes out of different


Maybe "NodeSelectors that do not consistently select the same set of nodes will make round-robin...."

nik9000 · 2018-06-21T13:10:26Z

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

+        return builder.build();
+    }
+
+    protected static void configureClient(RestClientBuilder builder, Settings settings) {


I'd prefer to declare the IOException and catch it at the call site if we don't want it rather than catch it here.

javanna · 2018-06-22T07:58:13Z

retest this please

* master: Add get field mappings to High Level REST API Client (#31423) [DOCS] Updates Watcher examples for code testing (#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (#31474) [DOCS] Move monitoring to docs folder (#31477) Core: Combine doExecute methods in TransportAction (#31517) IndexShard should not return null stats (#31528) fix repository update with the same settings but different type (#31458) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) Node selector per client rather than per request (#31471) Core: Combine messageRecieved methods in TransportRequestHandler (#31519) Upgrade to Lucene 7.4.0. (#31529) [ML] Add ML filter update API (#31437) Allow multiple unicast host providers (#31509) Avoid deprecation warning when running the ML datafeed extractor. (#31463) REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) [DOCS] Fix REST tests in SQL docs [DOCS] Add code snippet testing in more ML APIs (#31339) Core: Remove ThreadPool from base TransportAction (#31492) [DOCS] Remove fixed file from build.gradle Rename createNewTranslog to fileBasedRecovery (#31508) Test: Skip assertion on windows [DOCS] Creates field and document level security overview (#30937) [DOCS] Significantly improve SQL docs [DOCS] Move migration APIs to docs (#31473) Core: Convert TransportAction.execute uses to client calls (#31487) Return transport addresses from UnicastHostsProvider (#31426) Ensure local addresses aren't null (#31440) Remove unused generic type for client execute method (#31444) Introduce http and tcp server channels (#31446)

* elastic/master: (92 commits) Reduce number of raw types warnings (elastic#31523) Migrate scripted metric aggregation scripts to ScriptContext design (elastic#30111) turn GetFieldMappingsResponse to ToXContentObject (elastic#31544) Close xcontent parsers (partial) (elastic#31513) Ingest Attachment: Upgrade Tika to 1.18 (elastic#31252) TEST: Correct the assertion arguments order (elastic#31540) Add get field mappings to High Level REST API Client (elastic#31423) [DOCS] Updates Watcher examples for code testing (elastic#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (elastic#31474) [DOCS] Move monitoring to docs folder (elastic#31477) Core: Combine doExecute methods in TransportAction (elastic#31517) IndexShard should not return null stats (elastic#31528) fix repository update with the same settings but different type (elastic#31458) Fix Mockito trying to mock IOException that isn't thrown by method (elastic#31433) (elastic#31527) Node selector per client rather than per request (elastic#31471) Core: Combine messageRecieved methods in TransportRequestHandler (elastic#31519) Upgrade to Lucene 7.4.0. (elastic#31529) [ML] Add ML filter update API (elastic#31437) Allow multiple unicast host providers (elastic#31509) ...

We recently introduced a mechanism that allows to specify a node selector as part of do sections (see elastic#31471). When a node selector that is not the default one is configured, a new client will be initialized with the same properties as the default one, but with the specified node selector. This commit improves such mechanism but also closing the additional clients being created and adding equals/hashcode impl to the custom node selector as they are cached into a map.

) We recently introduced a mechanism that allows to specify a node selector as part of do sections (see #31471). When a node selector that is not the default one is configured, a new client will be initialized with the same properties as the default one, but with the specified node selector. This commit improves such mechanism but also closing the additional clients being created and adding equals/hashcode impl to the custom node selector as they are cached into a map.

We have made node selectors configurable per request, but all of other language clients don't allow for that. A good reason not to do so, is that having a different node selector per request breaks round-robin. This commit makes NodeSelector configurable only at client initialization. It also improves the docs on this matter, important given that a single node selector can still affect round-robin.

) We recently introduced a mechanism that allows to specify a node selector as part of do sections (see #31471). When a node selector that is not the default one is configured, a new client will be initialized with the same properties as the default one, but with the specified node selector. This commit improves such mechanism but also closing the additional clients being created and adding equals/hashcode impl to the custom node selector as they are cached into a map.

* 6.x: Docs: Remove duplicate test setup Docs: Fix description of percentile ranks example example (#31652) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Remove item listed in 6.3 notes that's not in 6.3 (#31623) remove unused import Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) QA: Merge query-builder-bwc to restart test (#30979) Docs: Skip xpack snippet tests if no xpack (#31619) [TEST] Fix RemoteClusterConnectionTests Remove legacy MetaDataStateFormat (#31603) [TEST] call yaml client close method from test suite (#31591) [TEST] Close additional clients created while running yaml tests (#31575) Node selector per client rather than per request (#31471) Preserve thread context when connecting to remote cluster (#31574) Remove redundant 'minimum_should_match' Unify headers for full text queries JDBC driver prepared statement set* methods (#31494) add logging breaking changes from 5.3 to 6.0 (#31583) Fix a formatting issue in the docvalue_fields documentation. (#31563) Improve robustness of geo shape parser for malformed shapes QA: Create xpack yaml features (#31403)

javanna added 2 commits June 20, 2018 16:56

Merge branch 'master' into enhancement/node_selectors

98e6ee6

javanna added >enhancement :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 v6.4.0 labels Jun 20, 2018

javanna requested a review from nik9000 June 20, 2018 16:38

Merge branch 'master' into enhancement/node_selectors

5497d33

nik9000 approved these changes Jun 21, 2018

View reviewed changes

javanna added 3 commits June 21, 2018 16:52

review comments

023166e

fix compile

9843b53

Merge branch 'master' into enhancement/node_selectors

4b1a5f8

Merge branch 'master' into enhancement/node_selectors

c7975f3

javanna merged commit 16e4e7a into elastic:master Jun 22, 2018

javanna added the backport pending label Jun 22, 2018

javanna mentioned this pull request Jun 26, 2018

[TEST] Close additional clients created while running yaml tests #31575

Merged

javanna removed the backport pending label Jun 27, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

matsumana mentioned this pull request Aug 25, 2020

[DOCS] Fix old NodeSelector field in Low Level REST Client #61551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node selector per client rather than per request#31471

Node selector per client rather than per request#31471
javanna merged 7 commits intoelastic:masterfrom
javanna:enhancement/node_selectors

javanna commented Jun 20, 2018

Uh oh!

elasticmachine commented Jun 20, 2018

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Jun 21, 2018

Uh oh!

nik9000 Jun 21, 2018

Uh oh!

javanna Jun 21, 2018

Uh oh!

nik9000 Jun 21, 2018

Uh oh!

javanna Jun 21, 2018

Uh oh!

nik9000 Jun 21, 2018

Uh oh!

javanna commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

javanna commented Jun 20, 2018

Uh oh!

elasticmachine commented Jun 20, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

nik9000 Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

javanna Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

nik9000 Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

javanna Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

nik9000 Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

javanna commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants