[PDS-403847] [III] Part 3: Accept Nodes Presenting Plausible-Evolution IDs Even If Not In Quorum #6768

jeremyk-91 · 2023-10-10T15:59:29Z

General

Before this PR:
Suppose we discover three new nodes in a Cassandra cluster when a quorum is not available. We currently attempt to get a quorum from them and then check if the topology they agree on is a plausible evolution of what the cluster has agreed on in the past - if so, we admit the nodes in the cluster that provided one of the topologies agreed upon by the quorum of new nodes. But this is unnecessary: we should not need a quorum of new nodes to be up if they agree with what we already have.

After this PR:
==COMMIT_MSG==
We now admit new nodes that match the existing topology even in the absence of a quorum of new nodes.
==COMMIT_MSG==

Priority: P2

Concerns / possible downsides (what feedback would you like?):

There's some room for debate on dissent when the cluster also had no quorum, though I viewed it as fine to add the non-dissenting nodes since the dissenting nodes could have been down anyway.
This could admittedly have been written as two parts but it's not too large overall so I opted not to do that.

Is documentation needed?: No

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: No

Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No

The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes

Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No

Does this PR need a schema migration? No

Testing and Correctness

What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing much

What was existing testing like? What have you done to improve it?: Added new tests

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: Rollback

If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: No

Execution

How would I tell this PR works in production? (Metrics, logs, etc.): Migrations work

Has the safety of all log arguments been decided correctly?: I believe so.

Will this change significantly affect our spending on metrics or logs?: No. There's a small risk in that we've noted before that logging CassandraServer is expensive, but we do it only once per refresh.

How would I tell that this PR does not work in production? (monitors, etc.): Migrations don't work

If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Rollback

If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: No

Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: No

Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: Don't think so

Development Process

Where should we start reviewing?: CTV

If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:

Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju

…db into jkong/403847-group-flames

…irection

mdaudali

looks fine, although one flag about hard failures

mdaudali · 2023-10-10T16:59:21Z

...assandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/IdSupportingHostIdResult.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface IdSupportingHostIdResult {


This is a confusing name, imo, but I don't have a good one offhand (it reads as if the host id supports IDs which is... odd, even though, from the exception message in check, I gathered that you mean that this includes results from a host that supports returning host ids)

Discussed offline: Let's go with NonSoftFailureHostIdResult. Your inference is correct, but we should make it explicit.

mdaudali · 2023-10-10T17:06:59Z

...sandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraTopologyValidator.java

-                    return newServersWithoutSoftFailures.keySet();
-                }
-                ConsistentClusterTopologies newNodesAgreedTopologies = maybeTopologies.get();
+                        .filterValues(result -> result.result().type() == HostIdResult.Type.SUCCESS


consider: consider having IdSupporting... have derived type and result fields so you don't have result.result().type() e.t.c?

yeah let's do this! I thought about it - was a bit hesitant to include the Type field because it was on HostIdResult, but these are pretty coupled anyway

mdaudali · 2023-10-10T17:09:11Z

...sandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraTopologyValidator.java

+                        .filterValues(result -> result.result().type() == HostIdResult.Type.SUCCESS
+                                && pastTopologies.sharesAtLeastOneHostId(
+                                        result.result().hostIds()))
+                        .toMap();
                ConsistentClusterTopologies pastTopologySnapshot = pastConsistentTopologies.get();


This line is now unnecessary (replaced with a hoist to L243)

womp - bad merge conflict resolution on my part. Thanks

mdaudali · 2023-10-10T17:14:07Z

...assandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/IdSupportingHostIdResult.java

+    @Value.Check
+    default void check() {
+        Preconditions.checkArgument(
+                result().type() != HostIdResult.Type.SOFT_FAILURE,


Do you need to exclude hard failures too? Since they too will have an empty set of hostIds

As discussed: this was more NonSoftFailureHostIdResult as opposed to a host ID result with IDs, so hard failures are allowed here. We could extract another class for the Success case, but given that that's pretty much just before we extract the host IDs and manipulate the Set<String> I think the marginal value is a bit lower (whereas the "we got rid of soft failures" spans a bigger section of the code).

mdaudali · 2023-10-10T17:17:03Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/HostIdEvolution.java

-            // connected by non-empty intersections.
-            return false;
-        }
+        Preconditions.checkArgument(!sets.contains(ImmutableSet.of()), "Empty sets of host ids are not allowed");


non-actionable: The existence of a type NonemptySet would be nice!

Yep, it would :)

mdaudali

👍

mdaudali · 2023-10-11T10:25:18Z

...sandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/NonSoftFailureHostIdResult.java

+    @Value.Check
+    default void check() {
+        Preconditions.checkArgument(
+                result().type() != HostIdResult.Type.SOFT_FAILURE,


the nittiest of nits, and I don't really care if you don't action this: you can just use type!

gah! this is my fault for not pushing changes all the way down

and, as per recent discussions, you can be very sure this will be actioned 😉

mdaudali · 2023-10-11T10:25:57Z

...ra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraTopologyValidatorTest.java

@@ -389,8 +389,8 @@ public void returnsAllNewHostsIfNoConsensusOnNewHostsWhenNoQuorumAndCurrentServe
        assertThat(hostsOffline).hasSizeGreaterThanOrEqualTo((ALL_HOSTS.size() + 1) / 2);
        assertThat(validator.getNewHostsWithInconsistentTopologies(
                        mapToTokenRangeOrigin(newCassandraServers), allHosts))
-                .as("rejects all servers when no quorum from all hosts and no quorum on new hosts")
-                .containsExactlyInAnyOrderElementsOf(newCassandraServers);
+                .as("accepts servers with plausible evolution when no quorum from all hosts and no quorum on new hosts")


the test name doesn't match the tested behaviour anymore

mdaudali · 2023-10-11T10:26:07Z

...ra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraTopologyValidatorTest.java

@@ -537,8 +537,9 @@ public void returnsAllNewHostsIfNoConsensusOnConfigHostsWhenNoQuorumAndNoCurrent
                        splitHostOriginBetweenLastKnownAndConfig(
                                oldCassandraServers, Sets.difference(allCassandraServers, oldCassandraServers)),
                        allHosts))
-                .as("rejects all servers when no quorum from all hosts and no quorum on available hosts")
-                .containsExactlyInAnyOrderElementsOf(allCassandraServers);
+                .as("accepts new servers with a plausible evolution when no quorum from all hosts and no quorum on"


likewise here

good catches! Will fix.

svc-autorelease · 2023-10-12T10:00:30Z

Released 0.954.0

jeremyk-91 and others added 18 commits October 9, 2023 17:54

updates

3e209ef

fix revapi

b234c5c

self cr

7ea5738

spotlesssssssssssssssss

93b6bea

update topology

a60075d

self cr

2dc84ff

fix

b511673

fork test

5e36394

spotless

7eeb1fb

Merge branch 'develop' into jkong/403847-group-flames

faf2287

Add generated changelog entries

c479009

brrrrrr

bfee3cb

Merge branch 'jkong/403847-group-flames' of github.com:palantir/atlas…

7a45687

…db into jkong/403847-group-flames

allow nonconsensus additions

c19d654

Merge remote-tracking branch 'origin/develop' into jkong/403847-one-d…

f64c861

…irection

Spotless

ff2bfe0

self cr

dff91b3

oops

0444103

mdaudali reviewed Oct 10, 2023

View reviewed changes

jeremyk-91 added 2 commits October 10, 2023 18:50

CR feedback

f4aa11e

self CR iteration 2

9751175

mdaudali approved these changes Oct 11, 2023

View reviewed changes

jeremyk-91 added 3 commits October 11, 2023 20:03

CR errors

39ff2cf

more than expected

092f7d8

changelog

7c8b257

jeremyk-91 added autorelease merge when ready labels Oct 12, 2023

bulldozer-bot bot merged commit 995bae6 into develop Oct 12, 2023

bulldozer-bot bot deleted the jkong/403847-one-direction branch October 12, 2023 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PDS-403847] [III] Part 3: Accept Nodes Presenting Plausible-Evolution IDs Even If Not In Quorum #6768

[PDS-403847] [III] Part 3: Accept Nodes Presenting Plausible-Evolution IDs Even If Not In Quorum #6768

jeremyk-91 commented Oct 10, 2023

mdaudali left a comment

mdaudali Oct 10, 2023

jeremyk-91 Oct 10, 2023

mdaudali Oct 10, 2023

jeremyk-91 Oct 10, 2023

mdaudali Oct 10, 2023

jeremyk-91 Oct 10, 2023

mdaudali Oct 10, 2023

jeremyk-91 Oct 10, 2023

mdaudali Oct 10, 2023

jeremyk-91 Oct 10, 2023

mdaudali left a comment

mdaudali Oct 11, 2023

jeremyk-91 Oct 11, 2023

jeremyk-91 Oct 11, 2023

mdaudali Oct 11, 2023

mdaudali Oct 11, 2023

jeremyk-91 Oct 11, 2023

svc-autorelease commented Oct 12, 2023

[PDS-403847] [III] Part 3: Accept Nodes Presenting Plausible-Evolution IDs Even If Not In Quorum #6768

[PDS-403847] [III] Part 3: Accept Nodes Presenting Plausible-Evolution IDs Even If Not In Quorum #6768

Conversation

jeremyk-91 commented Oct 10, 2023

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

mdaudali 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

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

mdaudali 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

Choose a reason for hiding this comment

svc-autorelease commented Oct 12, 2023