IGNITE-13204 Add Java thin client Kubernetes auto discovery #8206

timoninmaxim · 2020-09-01T16:08:03Z

ReliableChannel now works with dynamic list of server addresses;
ClientConfiguration can be configured with addressFinder;
Provide Kubernetes addressFinder.

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ClientChannelHolder.java

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ClientChannelHolder.java

...s/kubernetes/src/main/java/org/apache/ignite/kubernetes/KubernetesConnectorConfigurator.java

...g/apache/ignite/spi/discovery/tcp/ipfinder/kubernetes/ThinClientKubernetesAddressFinder.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

isapego

I got an impression that provided solution has a lot of non-necessary parts, like a lot of different holders and wrappers, and their purpose is unclean. Can you add a detailed description for new classes which explains why are they needed?

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ClientChannelHolder.java

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

...les/kubernetes/src/main/java/org/apache/ignite/client/ThinClientKubernetesAddressFinder.java

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/TcpIgniteClient.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

...les/kubernetes/src/main/java/org/apache/ignite/client/ThinClientKubernetesAddressFinder.java

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

...t/java/org/apache/ignite/internal/client/thin/ThinClientPartitionAwarenessDiscoveryTest.java

timoninmaxim · 2020-10-01T10:26:40Z

@ptupitsyn hi! I've merge your changes related to async thin client API. I leverage on introduced by me apply* methods and then remove AtomicInteger you're used for the counter, also I've fix exception issue (when final exception missed if it's different from ClientConnectionException after retry) and add test for that.

Could you please review my changes again.

modules/core/src/test/java/org/apache/ignite/internal/client/thin/ReliableChannelTest.java

modules/indexing/pom.xml

modules/core/src/main/java/org/apache/ignite/client/ClientAddressFinder.java

modules/indexing/pom.xml

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java

alex-plekhanov · 2020-10-14T07:16:58Z

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

+            chAndAttempts = applyOnDefaultChannel(channel -> channel, attemptsLimit);
+
+        } catch (Throwable ex) {
+            fut.completeExceptionally(ex);


What if failure already not null here? We will lose this information?

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

alex-plekhanov · 2020-10-14T07:17:42Z

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

+            .serviceAsync(op, payloadWriter, payloadReader)
+            .handle((res, err) -> {
+                if (err == null) {
+                    fut.complete(res);


Maybe you mean "Redundant NL"? Will fIx it this way

Ah, I see in next comments. There is already NL, will not change it.

No, I mean missing empty line between fut.complete(res); and onChannelFailure(ch);

Sorry, I don't see this case.

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

alex-plekhanov · 2020-10-14T14:28:10Z

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

+        if (scheduledChannelsReinit.get())
+            channelsInit(true);
+        else
+            rollCurrentChannel(hld);


Why do we skip rollCurrentChannel if scheduledChannelsReinit? I think we should roll channel in any case.

if channel fails and there is scheduled reinit, there are chances that reinitialization will fix the issue (reinitialization may assign new default channel).

There are chances if some dynamic address finder is used, but by default, you just check current address twice and skip one channel. In both cases (dynamic and static address finder) there is nothing bad happens if we roll current channel

Also, if new default channel is assigned by channelsInit(), than rollCurrentChannel(hld) will do no-op, since hld already not a default channel.

Actually there is an invokation sequence for default case: channelsInit -> applyOnDefaultChannel to establish connection with default channel (that checks that connection is OK). So rolling is not required too. Also rolling at least acquires lock so it is not free.

So, there is a matrix of cases for channel failure:

Fixed addreses, topology changes -> Channel is rolled with channelsInit -> applyOnDefaultChannel;

Fixed addresses, topology does not change -> rollCurrentChannel;

Dynamic address, topology changes -> channels are reinited twice, rolls with applyOnDefaultChannel;

Dynamic address, topology does not change -> rollCurrentChannel.

So, there is an issue with double initialization for dynamic addresses only. I will fix it by skipping second initialization as describe in other comment.

alex-plekhanov · 2020-10-14T14:31:50Z

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

+     * for every configured server. Otherwise only default channel is connected.
+     */
+    void channelsInit(boolean force) {
+        if (!force && channels != null)


Currently, flag force is useless (at least for production code, but perhaps for test code too). There are 3 calls of this method from production code, first - right after constructor (when channels == null), two other calls with force == true.

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

alex-plekhanov · 2020-10-14T14:36:18Z

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

-        }
+        chFailLsnrs.forEach(Runnable::run);
+
+        if (scheduledChannelsReinit.get())


Perhaps && !partitionAwarenessEnabled should be added here (to avoid double reinit for partition awareness, first time here and second time in async thread). WDYT?

Yeah, it is a correct optimization. But I will add this condition inside if block to avoid additional rolling of current channel.

...a/org/apache/ignite/internal/kubernetes/connection/KubernetesServiceAddressResolverTest.java

modules/kubernetes/pom.xml

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java

modules/kubernetes/src/test/java/org/apache/ignite/client/TestClusterClientConnection.java

...a/org/apache/ignite/internal/kubernetes/connection/KubernetesServiceAddressResolverTest.java

…rnetes_thin_client

timoninmaxim added 3 commits September 1, 2020 19:00

IGNITE-13204 Thin client kubernetes auto discovery

da60f72

fixup! IGNITE-13204 Thin client kubernetes auto discovery

4f6d1b1

fixup! IGNITE-13204 Thin client kubernetes auto discovery

003b164

ptupitsyn requested changes Sep 3, 2020

View reviewed changes

ptupitsyn changed the title ~~IGNITE-13204 Thin client kubernetes auto discovery~~ IGNITE-13204 Add Java thin client Kubernetes auto discovery Sep 3, 2020

ptupitsyn reviewed Sep 3, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java Outdated Show resolved Hide resolved

isapego reviewed Sep 3, 2020

View reviewed changes

timoninmaxim added 2 commits September 3, 2020 20:12

fixup! IGNITE-13204 Thin client kubernetes auto discovery

96655a1

fixup! IGNITE-13204 Thin client kubernetes auto discovery

f29d280

timoninmaxim requested a review from ptupitsyn September 3, 2020 17:47

fixup! IGNITE-13204 Thin client kubernetes auto discovery

36df449

ptupitsyn requested changes Sep 9, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java Outdated Show resolved Hide resolved

...les/kubernetes/src/main/java/org/apache/ignite/client/ThinClientKubernetesAddressFinder.java Outdated Show resolved Hide resolved

timoninmaxim added 3 commits September 10, 2020 08:53

fixup! IGNITE-13204 Thin client kubernetes auto discovery

6175a95

fixup! IGNITE-13204 Thin client kubernetes auto discovery

318bd12

fixup! IGNITE-13204 Thin client kubernetes auto discovery

431769f

ptupitsyn requested changes Sep 11, 2020

View reviewed changes

alex-plekhanov reviewed Sep 11, 2020

View reviewed changes

ptupitsyn reviewed Sep 11, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java Outdated Show resolved Hide resolved

alex-plekhanov reviewed Sep 11, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java Outdated Show resolved Hide resolved

alex-plekhanov reviewed Sep 14, 2020

View reviewed changes

...les/kubernetes/src/main/java/org/apache/ignite/client/ThinClientKubernetesAddressFinder.java Show resolved Hide resolved

fixup! IGNITE-13204 Thin client kubernetes auto discovery

6c8629b

timoninmaxim requested a review from ptupitsyn September 15, 2020 13:40

alex-plekhanov reviewed Sep 16, 2020

View reviewed changes

fixup! IGNITE-13204 Thin client kubernetes auto discovery

cbd0dde

timoninmaxim force-pushed the IGNITE-13204_kubernetes_thin_client branch from aea28f1 to cbd0dde Compare September 24, 2020 11:51

timoninmaxim added 2 commits September 24, 2020 15:16

fixup! IGNITE-13204 Thin client kubernetes auto discovery

99a519d

fixup! IGNITE-13204 Thin client kubernetes auto discovery

f0387c9

timoninmaxim force-pushed the IGNITE-13204_kubernetes_thin_client branch from 141524c to beebf80 Compare September 30, 2020 10:08

Merge branch 'master' into IGNITE-13204_kubernetes_thin_client

55f56c1

timoninmaxim force-pushed the IGNITE-13204_kubernetes_thin_client branch from beebf80 to 55f56c1 Compare September 30, 2020 12:59

Fix codestyle

7a32fe7

ptupitsyn requested changes Oct 6, 2020

View reviewed changes

Provide more javadocs.

73adfc5

ptupitsyn reviewed Oct 7, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/client/ClientAddressFinder.java Outdated Show resolved Hide resolved

ptupitsyn reviewed Oct 7, 2020

View reviewed changes

modules/core/src/main/java/org/apache/ignite/client/ClientAddressFinder.java Show resolved Hide resolved

modules/indexing/pom.xml Show resolved Hide resolved

Fix misprint

4b06038

alex-plekhanov reviewed Oct 14, 2020

View reviewed changes

timoninmaxim added 3 commits October 14, 2020 23:54

Add more new lines. Refactor getting attemps value

c03c88c

WIP

8203de1

Add kubernetes tests

2698804

alex-plekhanov reviewed Oct 16, 2020

View reviewed changes

Fix review comments.

86e5849

alex-plekhanov approved these changes Oct 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into IGNITE-13204_kube…

1c429a6

…rnetes_thin_client

ptupitsyn approved these changes Oct 16, 2020

View reviewed changes

ptupitsyn merged commit fa9c483 into apache:master Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-13204 Add Java thin client Kubernetes auto discovery #8206

IGNITE-13204 Add Java thin client Kubernetes auto discovery #8206

timoninmaxim commented Sep 1, 2020

isapego left a comment

timoninmaxim commented Oct 1, 2020

alex-plekhanov Oct 14, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 15, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 14, 2020

timoninmaxim Oct 14, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 14, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 14, 2020

alex-plekhanov Oct 14, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 14, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 15, 2020

alex-plekhanov Oct 14, 2020

timoninmaxim Oct 14, 2020

timoninmaxim Oct 15, 2020

IGNITE-13204 Add Java thin client Kubernetes auto discovery #8206

IGNITE-13204 Add Java thin client Kubernetes auto discovery #8206

Conversation

timoninmaxim commented Sep 1, 2020

isapego left a comment

Choose a reason for hiding this comment

timoninmaxim commented Oct 1, 2020

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

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

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