NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS #32764

original-brownbear · 2018-08-10T04:28:59Z

Lazy resolve DNS (i.e. String to DiscoveryNode) to not run into indefinitely caching lookup issues (provided the JVM dns cache is configured correctly as explained in https://www.elastic.co/guide/en/elasticsearch/reference/6.3/networkaddress-cache-ttl.html)
- Changed InetAddress type to String for that higher up the stack
- Passed down Supplier<DiscoveryNode> instead of outright DiscoveryNode from RemoteClusterAware#buildRemoteClustersSeeds on to lazy resolve DNS when the DiscoveryNode is actually used (could've also passed down the value of clusterName = REMOTE_CLUSTERS_SEEDS.getNamespace(concreteSetting) together with the List<String> of hosts, but this route seemed to introduce less duplication and resulted in a significantly smaller changeset).
Closes Remote cluster DNS cached indefinitely #28858

elasticmachine · 2018-08-10T04:29:00Z

Pinging @elastic/es-core-infra

chernecov · 2018-08-10T11:37:07Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

@@ -48,8 +49,12 @@
    /**
     * A list of initial seed nodes to discover eligible nodes from the remote cluster
     */
-    public static final Setting.AffixSetting<List<InetSocketAddress>> REMOTE_CLUSTERS_SEEDS = Setting.affixKeySetting("search.remote.",
-        "seeds", (key) -> Setting.listSetting(key, Collections.emptyList(), RemoteClusterAware::parseSeedAddress,
+    public static final Setting.AffixSetting<List<String>> REMOTE_CLUSTERS_SEEDS = Setting.affixKeySetting("search.remote.",


I am stranger here, so sorry if I am wrong, but it would be more readable:

public static final Setting.AffixSetting<List<String>> REMOTE_CLUSTERS_SEEDS = Setting.affixKeySetting( "search.remote.", "seeds", (key) -> Setting.listSetting( key, Collections.emptyList(), s -> { // validate seed address RemoteClusterAware.parseSeedAddress(s); return s; }, Setting.Property.NodeScope, Setting.Property.Dynamic ) );

No worries, I don't think there's much of a wrong or right here :) I tend to agree with your suggested style but if you look around the codebase you will find that my style is what's used pretty much everywhere so I went with the consistent approach here.

Huh, I see now.
That's completely fine then.
Consistency is a very good thing, your're right.
Thanks.

@original-brownbear I use the style suggested by @chernecov. 😇

Hehe that's all I needed to hear :) Will reformat

jasontedor

I skimmed this and the general approach looks right. I think what is missing though, sorry if I overlooked, is a test that shows that we are now resolve addresses on demand? That is, that at least we (independent of what the JVM might be doing), no longer cache addresses.

original-brownbear · 2018-08-15T05:44:18Z

@jasontedor thanks for taking a look :)

You didn't overlook anything, I was a bad one for not adding a test => added one now though :)
=> This should be good for another look.

original-brownbear · 2018-08-15T17:04:55Z

hold off a few minutes with the review, seems the new test is unstable in some way -> fixing

original-brownbear · 2018-08-15T17:48:26Z

@jasontedor should be stable/good to review now. Sorry for the noise :)

jasontedor

I have a concern about how we validate the seed node setting now.

jasontedor · 2018-08-16T16:36:19Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

+            key, Collections.emptyList(),
+            s -> {
+                // validate seed address
+                RemoteClusterAware.parseSeedAddress(s);


So this seems problematic. It means, for example, if a host fails to resolve, we mark the setting as invalid. Yet, it could resolve for us later?

@jasontedor yea, I guess we could leave it out. I mainly just added this because there was an explicit test for this behavior.
I guess we could just delete it and drop the validation. I could go either way here:
Yes this could resolve later, so maybe it's fine not to validate. On the other hand, you could make an annoying typo in a hostname and now you won't catch it right away. Probably a question of how often and if someone would ever configure a hostname that doesn't resolve yet. Also, if you don't have the DNS caching ttl configured, not having this check is kind of annoying because it will never resolve and you might rather want this to fail right away
=> You decide :)

If a hostname fails to resolve at time of use (i.e., when we try to connect to the remote cluster), we would have a failure message in the logs indicating the problem? That is, the unknown host exception would be there?

I lean towards saying we leave the lookup out of validation, yet I think the remainder of the validation is good.

@jasontedor sounds good will adjust accordingly in a bit :)

@jasontedor done in a22994a, extracted the port parsing from the address parsing to get all validation except for the hostname lookup into a separate method :)

jasontedor

LGTM. I left a few comments, but no need for another round.

jasontedor · 2018-08-17T13:35:57Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

            if (port <= 0) {
                throw new IllegalArgumentException("port number must be > 0 but was: [" + port + "]");
            }
-            return new InetSocketAddress(hostAddress, port);
+            return port;
        } catch (NumberFormatException e) {
            throw new IllegalArgumentException("port must be a number", e);


I think this error message can be misleading. For example, Integer.valueOf will fail to parse 123456789123456789 with a NumberFormatException yet 123456789123456789 is a number! I am okay with catching NumberFormatException and re-throwing since we can add context that NumberFormatException does not provide (For input string: "123456789123456789" is not helpful, specifying that this occurred while parsing a port is) but I think a more correct error message would be helpful. For example, failed to parse port.

Also, I do not see a test for this case.

jasontedor · 2018-08-17T13:45:41Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

-                        transportAddress,
-                        Version.CURRENT.minimumCompatibilityVersion());
-                    nodes.add(node);
+                List<Supplier<DiscoveryNode>> nodes = new ArrayList<>();


We can pre-size the list?

sure will do :)

jasontedor

Oh, one more nit.

jasontedor · 2018-08-17T15:23:08Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

+                parsePort(s);
+                return s;
+            },
+            Setting.Property.NodeScope, Setting.Property.Dynamic


These could probably be on a separate line each, too. 😇

sure will do :)

original-brownbear · 2018-08-17T17:13:53Z

@jasontedor thanks addressed all points => will merge once green :)

original-brownbear · 2018-08-17T17:41:54Z

Jenkins test this

* master: NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764) [DOCS] Splits the users API documentation into multiple pages (elastic#32825) [DOCS] Splits the token APIs into separate pages (elastic#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (elastic#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (elastic#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (elastic#32901) Make Geo Context Mapping Parsing More Strict (elastic#32821)

* elastic/master: (46 commits) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (#32764) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Make Geo Context Mapping Parsing More Strict (#32821) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Scripted metric aggregations: add deprecation warning and system property to control legacy params (#31597) Tests: Fix timezone conversion in DateTimeUnitTests Enable FIPS140LicenseBootstrapCheck (#32903) Fix InternalAutoDateHistogram reproducible failure (#32723) Remove assertion in testDocStats on deletedDocs counter (#32914) HLRC: Move ML request converters into their own class (#32906) ...

* Lazy resolve DNS (i.e. `String` to `DiscoveryNode`) to not run into indefinitely caching lookup issues (provided the JVM dns cache is configured correctly as explained in https://www.elastic.co/guide/en/elasticsearch/reference/6.3/networkaddress-cache-ttl.html) * Changed `InetAddress` type to `String` for that higher up the stack * Passed down `Supplier<DiscoveryNode>` instead of outright `DiscoveryNode` from `RemoteClusterAware#buildRemoteClustersSeeds` on to lazy resolve DNS when the `DiscoveryNode` is actually used (could've also passed down the value of `clusterName = REMOTE_CLUSTERS_SEEDS.getNamespace(concreteSetting)` together with the `List<String>` of hosts, but this route seemed to introduce less duplication and resulted in a significantly smaller changeset). * Closes elastic#28858

* Lazy resolve DNS (i.e. `String` to `DiscoveryNode`) to not run into indefinitely caching lookup issues (provided the JVM dns cache is configured correctly as explained in https://www.elastic.co/guide/en/elasticsearch/reference/6.3/networkaddress-cache-ttl.html) * Changed `InetAddress` type to `String` for that higher up the stack * Passed down `Supplier<DiscoveryNode>` instead of outright `DiscoveryNode` from `RemoteClusterAware#buildRemoteClustersSeeds` on to lazy resolve DNS when the `DiscoveryNode` is actually used (could've also passed down the value of `clusterName = REMOTE_CLUSTERS_SEEDS.getNamespace(concreteSetting)` together with the `List<String>` of hosts, but this route seemed to introduce less duplication and resulted in a significantly smaller changeset). * Closes #28858

* commit '9088d811ce9cff922e6ef1befbeb0f1e0c27016a': (23 commits) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) HLREST: AwaitsFix ML Test Make Geo Context Mapping Parsing More Strict (elastic#32862) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Fix use of deprecated apis Only configure publishing if it's applied externally (elastic#32351) Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) Scripted metric aggregations: add deprecation warning and system (elastic#32944) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) Set forbidden APIs target compatibility to compiler java version (elastic#32935) [TEST] Add "ne" as an unsupported SimpleKdc locale (elastic#32700) MINOR: Remove `IndexTemplateFilter` (elastic#32841) (elastic#32974) INGEST: Create Index Before Pipeline Execute (elastic#32786) (elastic#32975) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764) (elastic#32976) ...

original-brownbear added 4 commits August 9, 2018 13:24

28858 works

fb9d964

Merge remote-tracking branch 'elastic/master' into 28858

f68901a

Merge remote-tracking branch 'elastic/master' into 28858

11a1f4b

Merge remote-tracking branch 'elastic/master' into 28858

290dab0

original-brownbear added >bug :Distributed/Network Http and internode communication implementations v7.0.0 v6.5.0 labels Aug 10, 2018

original-brownbear mentioned this pull request Aug 10, 2018

Remote cluster DNS cached indefinitely #28858

Closed

chernecov reviewed Aug 10, 2018

View reviewed changes

original-brownbear requested a review from jasontedor August 13, 2018 21:30

jasontedor requested changes Aug 15, 2018

View reviewed changes

original-brownbear added 3 commits August 15, 2018 07:12

merge in master

774fe5e

CR: Reformat

d660135

CR: Add test

11997c3

original-brownbear added 2 commits August 15, 2018 19:05

Merge remote-tracking branch 'elastic/master' into 28858

2b64312

Stabilize test

b3eb225

jasontedor requested changes Aug 16, 2018

View reviewed changes

original-brownbear added 2 commits August 16, 2018 19:10

Merge remote-tracking branch 'elastic/master' into 28858

f2e8f9b

CR: Only validate form of seed address not hostname lookup

a22994a

jasontedor approved these changes Aug 17, 2018

View reviewed changes

jasontedor reviewed Aug 17, 2018

View reviewed changes

original-brownbear added 2 commits August 17, 2018 18:49

Merge remote-tracking branch 'elastic/master' into 28858

ced240b

CR: Nits

1114e17

original-brownbear added 3 commits August 17, 2018 20:00

Merge remote-tracking branch 'elastic/master' into 28858

8d10c13

Merge remote-tracking branch 'elastic/master' into 28858

f6b75c3

Merge remote-tracking branch 'elastic/master' into 28858

b86024f

original-brownbear merged commit f82bb64 into elastic:master Aug 18, 2018

original-brownbear deleted the 28858 branch August 18, 2018 06:46

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS #32764

NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS #32764

original-brownbear commented Aug 10, 2018 •

edited

Loading

elasticmachine commented Aug 10, 2018

chernecov Aug 10, 2018 •

edited

Loading

original-brownbear Aug 10, 2018

chernecov Aug 10, 2018

jasontedor Aug 15, 2018

original-brownbear Aug 15, 2018

jasontedor left a comment

original-brownbear commented Aug 15, 2018

original-brownbear commented Aug 15, 2018

original-brownbear commented Aug 15, 2018

jasontedor left a comment

jasontedor Aug 16, 2018

original-brownbear Aug 16, 2018

jasontedor Aug 16, 2018

original-brownbear Aug 16, 2018

original-brownbear Aug 16, 2018

jasontedor left a comment

jasontedor Aug 17, 2018

jasontedor Aug 17, 2018

jasontedor Aug 17, 2018

original-brownbear Aug 17, 2018

jasontedor left a comment

jasontedor Aug 17, 2018

original-brownbear Aug 17, 2018

original-brownbear commented Aug 17, 2018

original-brownbear commented Aug 17, 2018

NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS #32764

NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS #32764

Conversation

original-brownbear commented Aug 10, 2018 • edited Loading

elasticmachine commented Aug 10, 2018

chernecov Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

original-brownbear commented Aug 15, 2018

original-brownbear commented Aug 15, 2018

original-brownbear commented Aug 15, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Aug 17, 2018

original-brownbear commented Aug 17, 2018

original-brownbear commented Aug 10, 2018 •

edited

Loading

chernecov Aug 10, 2018 •

edited

Loading