JAVA-1388: Add dynamic port discovery for system.peers_v2 #1065

tolbertam · 2018-07-29T02:16:56Z

Based on #1064 for now until that is merged.

I followed the approaches in #1000 where we query the peers_v2 table first, and if that fails, downgrade to the peers table and never query v2 again. I just realized we should probably be more selective about downgrading based on the error returned by that (both here and on 4.x), as its reasonable to expect we can get other errors (like client timeouts) that don't necessitate downgrading to the peers table.

I updated ScassandraCluster to support multiple nodes per IP and added a test for this, and it seems to work well. It would be nice to have tests against real cassandra nodes, which will require CCM changes, which may take me a little bit. Also needs tests for the broadcast address changing / query of peers table for a single host that we currently have in ControlConnectionTest for the existing peers table.

tolbertam · 2018-07-30T16:14:14Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

+   * @return result of peers query.
+   */
+  private ListenableFuture<ResultSet> selectPeersFuture(final Connection connection) {
+    if (isPeersV2) {


For < C* 4.x, maybe we should just not even try the peers_v2 table. We could only allow this for protocol V5+. I think that's an ok limitation, and we can add some kind of way to bypass the V5 requirement for our Scassandra tests.

Thinking about this more. I think for 3.x, this should be more of an opt-in thing via a builder option. For a minor version making an extra query on initialization seems kinda iffy, and I think it's ok to require use of this feature to be explicit, since the 3.x driver is more driven around ip addresses and global setting of port (via withPort), so we should probably make a separate option so users can opt-in to this capability.

We discussed this more and decided that it's ok to try peers_v2 table first.

tolbertam · 2018-07-30T22:00:07Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

+   * @return result of peers query.
+   */
+  private ListenableFuture<ResultSet> selectPeersFuture(final Connection connection) {
+    String query =


This is very explicit now, the peers_v2 is only ever queried if allowHostPortDiscovery is used, and if the query fails, cluster initialization fails.

One thing to think about here is whether or not we should fall back to the old system peers query if we get an invalid query request back. I think in that specific case it might be sane to fall back on old peers query.

I've wavered back and forth on this, my current line of thinking is if the user opts in using allowHostPortDiscovery, they are intending to depend on the presence of the peers_v2 table to tell them what ports to use. In absence of that table, I think failing is the right thing to do. Whereas with java driver 4.x, we always check this table first, so in that case it makes sense to downgrade to the peers table.

adutra · 2018-08-01T12:56:34Z

driver-core/src/main/java/com/datastax/driver/core/Cluster.java


+    /**
+     * Enables host port discovery using the system.peers_v2 table added in Cassandra 4.0 (via
+     * CASSANDRA-7544). This enables running multiple Cassandra


Seems like the last sentence is truncated.

woops, might have accidentally triggered a vim hotkey 😆 will fix.

adutra · 2018-08-01T13:02:35Z

driver-core/src/main/java/com/datastax/driver/core/Cluster.java

+     *
+     * @return this builder.
+     */
+    public Builder allowHostPortDiscovery() {


Why expose this? Can't we simply detect the presence of the new peers_v2 table? Or is it because if the cluster has some nodes with peers_v2 and others without, that might be a too complex situation to handle?

I explain my rationale for this here: #1065 (comment). I originally tried peers_v2 first, then peers if the table doesn't exist (like 4.x does). But thinking about it more I wasn't sure if adding another query in a minor version was the right thing to do, so I thought maybe making this an Opt-in feature on 3.x would be the right way to go.

Doesn't sound that bad to add that extra query, since it's done only once at the beginning, and if that fails then we always go directly to the legacy table.

So are we doing the fallback to system.peers at all anymore? The PR description says so...

I was erring on the side of caution by making this an 'opt-in' feature. If you don't see that as too risky, I will revert it back to my initial implementation which mirrors the approach we took with 4.x. With regards to not falling back to system.peers, my thought process was that if you explicitly opted in using allowHostPortDiscovery(), it didn't seem the backing off to peers was the best idea, since you are expressing that you want to utilize the capability which is not being provided by the server. If i revert to not making this opt-in that point is moot though.

adutra · 2018-08-01T13:04:55Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

+    InetSocketAddress broadcastAddress = null;
    if (row.getColumnDefinitions().contains("peer")) { // system.peers
-      broadcastAddress = row.getInet("peer");
+      int broadcastPort =


Instead of zero, shouldn't we use the default port number (9042)?

This is for the broadcast/listen address which is typically 7000. I felt that since we can't know for sure what it's configured to, using 0 would be better.

Oh sorry, of course. 👍

adutra · 2018-08-01T13:08:10Z

driver-core/src/main/java/com/datastax/driver/core/Host.java

+   *     href="https://docs.datastax.com/en/cassandra/2.1/cassandra/configuration/configCassandra_yaml_r.html">The
+   *     cassandra.yaml configuration file</a>
+   */
+  public InetAddress getBroadcastAddress() {


This method is now used only in tests. Are you keeping it for binary compatibility? In this case we might want to mark it @Deprecated just as you did for getListenAddress().

I originally marked these methods deprecated, but then I backed off when I realized for most users the InetSocketAddress version will return port 0.

Makes sense but then for consistency maybe we should not mark getListenAddress() as deprecated?

D'oh! Didn't realize that, I'll make them consistent, thanks.

We should at least explain that it duplicates information already present in the other method.

adutra · 2018-08-01T13:09:03Z

driver-core/src/main/java/com/datastax/driver/core/Host.java

-   * Returns the node listen address (that is, the IP the node uses to contact other peers in the
-   * cluster), if known.
+   * @return the node listen address, if known. Otherwise {@code null}.
+   * @deprecated


You might want to explain why it is deprecated ("Use #getListenSocketAddress() instead").

GregBestland

+1 looks good one Just one small thing to consider. Merge at your discretion

GregBestland · 2018-08-02T20:46:58Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

+   * @return result of peers query.
+   */
+  private ListenableFuture<ResultSet> selectPeersFuture(final Connection connection) {
+    String query =


One thing to think about here is whether or not we should fall back to the old system peers query if we get an invalid query request back. I think in that specific case it might be sane to fall back on old peers query.

tolbertam · 2018-08-03T14:55:16Z

Tests surfaced some very minor issues (log message contents, etc.) that i need to work out. will do.

tolbertam · 2018-08-03T18:38:06Z

rebased and fixed/enhanced tests.

tolbertam · 2018-08-06T18:23:41Z

Rebased on 3.x

olim7t

I'm not really convinced we should make this opt-in. If you connect to a Cassandra 4 cluster with the driver, things should just work.

olim7t · 2018-08-15T17:24:06Z

driver-core/src/main/java/com/datastax/driver/core/Cluster.java

+     *
+     * @return this builder.
+     */
+    public Builder allowHostPortDiscovery() {


Doesn't sound that bad to add that extra query, since it's done only once at the beginning, and if that fails then we always go directly to the legacy table.

olim7t · 2018-08-15T17:58:39Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

                + "(this can happen if the broadcast address changed)",
-            host.getBroadcastAddress(),
+            address,
            host);


I'm a bit lost here: why do we read broadcastSocketAddress again?
addressToUse is never used.
address is only used in the log message.

i think this is an artifact of some refactoring, i'll fix this.

olim7t · 2018-08-15T18:01:12Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

@@ -594,20 +614,26 @@ private static void updateInfo(
    // After CASSANDRA-9603 (2.0.17, 2.1.8, 2.2.0 rc2) local row contains one more column:
    // - listen_address


Maybe update this kind of comments to explain the new rows in peers_v2 (or remove them / have a single comment detailing the table formats in different versions?).

olim7t · 2018-08-15T18:11:49Z

driver-core/src/main/java/com/datastax/driver/core/Cluster.java

+     *
+     * @return this builder.
+     */
+    public Builder allowHostPortDiscovery() {


So are we doing the fallback to system.peers at all anymore? The PR description says so...

olim7t · 2018-08-15T18:17:06Z

driver-core/src/main/java/com/datastax/driver/core/Host.java

  // that's the 'peer' in the 'System.peers' table and avoids querying the full peers table in
  // ControlConnection.refreshNodeInfo.
-  private volatile InetAddress broadcastAddress;
+  private volatile InetSocketAddress broadcastAddress;


Could we rename this and the corresponding setter with Socket?
Right now we have:

getBroadcastAddress returning an InetAddress

setBroadcastAddress taking an InetSocketAddress

I'll rename the setters to *SocketAddress for consistency.

olim7t · 2018-08-15T18:19:13Z

driver-core/src/main/java/com/datastax/driver/core/Host.java

+   *     href="https://docs.datastax.com/en/cassandra/2.1/cassandra/configuration/configCassandra_yaml_r.html">The
+   *     cassandra.yaml configuration file</a>
+   */
+  public InetAddress getBroadcastAddress() {


We should at least explain that it duplicates information already present in the other method.

…ry fails. * Remove deprecated annotation from Host.getListenAddress * Fix javadoc for allowHostPortDiscovery * Throw a specialized error indicating that Cluster.Builder.allowHostPortDiscovery requires peers_v2.

* Switch back to the approach where peers_v2 is tried and fall back to system.peers if it doesn't exist instead of making opt-in via allowHostPortDiscovery. * rename set*Address methods to set*SocketAddress. * update comments as suggested.

tolbertam · 2018-08-15T20:22:52Z

Rebased on 3.x and processed feedback, please let me know if I missed anything. The behavior is now 1:1 with 4.x. The peers_v2 table will be queried initially, and if we get an InvalidQueryException, we fall back to the peers table and logic.

olim7t · 2018-08-15T21:40:45Z

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

+                : SELECT_PEERS
+                    + " WHERE peer='"
+                    + broadcastAddress.getAddress().getHostAddress()
+                    + "'";


We don't handle a missing peers_v2 here, but that's fine: we do a refreshNodeListAndTokenMap every time the control connection (re)connects, and it happens before the new connection is published in connectionRef, so we know it will never race with this method.
So isPeersV2 will always be set correctly at this point. Even in a mixed 3.x/4.0 cluster, if we initially connected to a 4.0 node, isPeersV2 will downgrade correctly if we reconnect to a legacy node.

- use the autodiscovery or ports made available in apache/cassandra-java-driver#1065

tolbertam added this to the 3.6.0 milestone Jul 29, 2018

tolbertam requested review from GregBestland and olim7t July 29, 2018 02:16

tolbertam commented Jul 30, 2018

View reviewed changes

adutra requested changes Aug 1, 2018

View reviewed changes

GregBestland approved these changes Aug 2, 2018

View reviewed changes

tolbertam force-pushed the java1786 branch from 635e5b7 to 1159641 Compare August 3, 2018 13:13

tolbertam force-pushed the java1388 branch from 0442e46 to 6a58030 Compare August 3, 2018 18:37

adutra approved these changes Aug 6, 2018

View reviewed changes

tolbertam changed the base branch from java1786 to 3.x August 6, 2018 18:19

tolbertam force-pushed the java1388 branch from 6a58030 to 9d7fcf6 Compare August 6, 2018 18:23

olim7t mentioned this pull request Aug 15, 2018

JAVA-1388: DO NOT MERGE: Add support for clusters with different native and storage ports at each node for CASSANDRA-7544 #931

Closed

olim7t suggested changes Aug 15, 2018

View reviewed changes

tolbertam added 7 commits August 15, 2018 15:11

JAVA-1388: Add dynamic port discovery for system.peers_v2

367d72f

[fixup] Only downgrade to peers table on InvalidQueryException

801bbc4

[fixup] Add allowHostPortDiscovery() builder option

9e1d85a

[fixup] process feedback and return specialized error if peers_v2 que…

15a444f

…ry fails. * Remove deprecated annotation from Host.getListenAddress * Fix javadoc for allowHostPortDiscovery * Throw a specialized error indicating that Cluster.Builder.allowHostPortDiscovery requires peers_v2.

[fixup] Adapt tests for peers_v2 table

fac2458

[fixup] Process feedback

27ea31f

* Switch back to the approach where peers_v2 is tried and fall back to system.peers if it doesn't exist instead of making opt-in via allowHostPortDiscovery. * rename set*Address methods to set*SocketAddress. * update comments as suggested.

[fixup] Use GuavaCompatibility where applicable.

bb60a75

tolbertam force-pushed the java1388 branch from 9d7fcf6 to bb60a75 Compare August 15, 2018 20:13

[fixup] Return invalid for peers_v2 request in ClusterInitTest

8ca0826

olim7t reviewed Aug 15, 2018

View reviewed changes

olim7t approved these changes Aug 15, 2018

View reviewed changes

tolbertam merged commit 598c5cb into 3.x Aug 15, 2018

tolbertam deleted the java1388 branch August 16, 2018 00:24

michaelsembwever added a commit to thelastpickle/cassandra that referenced this pull request Sep 21, 2019

WIP –– update to cassandra-java-driver-core-3.7.2

15f160e

- use the autodiscovery or ports made available in apache/cassandra-java-driver#1065

		@@ -594,20 +614,26 @@ private static void updateInfo(
		// After CASSANDRA-9603 (2.0.17, 2.1.8, 2.2.0 rc2) local row contains one more column:
		// - listen_address

JAVA-1388: Add dynamic port discovery for system.peers_v2 #1065

JAVA-1388: Add dynamic port discovery for system.peers_v2 #1065

Uh oh!

Conversation

tolbertam commented Jul 29, 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!

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!

tolbertam Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

GregBestland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolbertam commented Aug 3, 2018

Uh oh!

tolbertam commented Aug 3, 2018

Uh oh!

tolbertam commented Aug 6, 2018

Uh oh!

olim7t 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!

tolbertam Aug 15, 2018 •

edited

Loading