Remove token range checks, validate new servers on their topology #6317

Sam-Kramer · 2022-10-25T14:23:12Z

General

Before this PR:
On start up and when a host was added/removed, we would perform a 'sanity check' that all hosts in the cluster had the same token ring. This was a flaky enough of a call to warrant it being removed, nor did it actually prevent anything anyways.

After this PR:
Remove all token ring validation checks. Validate that any new host added to our cassandra pool see's all other hosts; this ensures that we do not accidentally introduce user-led split brain which would have been previously caught by the token ring validation checks.
==COMMIT_MSG==
Removes token ring validation checks and instead checks for user-led split brain by ensuring any host added to our connection pool has its topology validated.
==COMMIT_MSG==

Priority: P1

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

CassandraClientPool is a mess, and as a result this somewhat degraded this PR
API BREAKS, it doesn't look the classes I changed are exposed anywhere, but loved to be proved wrong :)

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?:
Potentially -- we can easily retain our old APIs if needed, but this should be an internal API.

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

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)?:
Yes (requires https://github.com/palantir/cassandra/releases/tag/1.101.0), and this is achieved by not performing topology validation when the endpoint is not present (i.e. we programmatically handle this case). It may be worth bumping:

atlasdb/atlasdb-cassandra/build.gradle

Line 128 in 284da7e

minimumVersion = '3.31.0'

, but this hasn't been done in a very long time!

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?:
None! We assume Cassandra hosts may have or not have support this endpoint, and support both cases.

What was existing testing like? What have you done to improve it?:
Added tests to ensure we do not add nodes with invalid topologies; also added tests to CassandraClientPool to ensure we re-use cassandra containers.

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:
N/A -- code is modifying a statement that is called by a synchronized method, and is the only method that is called to modify/change the pool.

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

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):
Metrics and logs. CassandraTopologyValidationMetrics can be used to determine how often this is failing fleetwide, and also log will contain the exact failures and differences.

Has the safety of all log arguments been decided correctly?:
Yes

Will this change significantly affect our spending on metrics or logs?:
No

How would I tell that this PR does not work in production? (monitors, etc.):
Service fails to start up during B/G upgrade, and we should create a monitor for tracking if validationFailures ever exceeds 0.

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?:
No

Development Process

Where should we start reviewing?:
Now!

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

Sam-Kramer · 2022-10-25T15:23:42Z

atlasdb-commons/src/main/java/com/palantir/util/MapUtils.java

+
+    private MapUtils() {}
+
+    public static <K, V> Map<K, V> combineMaps(Map<K, V> primary, Map<K, V> secondary) {


Couldn't find anything, but worth the reviewer double-checking / informing if there is something already :)

If there isn't, will write tests and add docs.

(Quick drive by before I actually review later) This looks to be shallow merging maps right? In that case you can use https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#merge-K-V-java.util.function.BiFunction-

Yeah I saw that, the only issue with this is that it modifies the underlying map

Another option is ImmutableMap.putAll(map2).putAll(map1).buildKeepingLast()?

Sam-Kramer · 2022-10-25T16:09:44Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

@@ -602,4 +561,9 @@ public enum StartupChecks {
        RUN,
        DO_NOT_RUN
    }
+
+    @VisibleForTesting
+    CassandraAbsentHostTracker getAbsentHostTracker() {


ugh this is ugly, but the alternative is to have this be passed in via constructor -- we can go down this path, but the constructor of this class is already bloated as it is.

alternatively we could abandon this altogether, and just not test absent host tracking (which was the case before!!)

Sam-Kramer · 2022-10-25T16:11:08Z

...b-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/pool/CassandraService.java

    }

-    public void returnOrCreatePool(CassandraServer server, Optional<CassandraClientPoolingContainer> container) {


My understanding is that users do not directly use this class.. therefore should not be an API break?

internal code search tool says there's no use, so we should be good :)

mdaudali

looking good! Something to consider for resource management of containers and some nits to make this easier to read.

I'll validate ABI breaks in the next pass, but they should be easy to fix if they exist :)

mdaudali · 2022-10-31T11:24:17Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

@@ -376,7 +376,10 @@ private Set<CassandraServer> validateNewHostsTopologiesAndMaybeAddToPool(Set<Cas
                getContainerForNewServers(serversToAdd);

        Map<CassandraServer, CassandraClientPoolingContainer> allContainers =
-                MapUtils.combineMaps(getCurrentPools(), serversToAddContainers);
+                ImmutableMap.<CassandraServer, CassandraClientPoolingContainer>builder()
+                        .putAll(getCurrentPools())


This is different to what you previously had. With combineMaps, the code kept the entry in the getCurrentPools map in the case of a duplicate between GCP and STAC. In the new code, you prefer STAC entries in the case of a duplicate (hence why I mentioned map2 then map1, rather than the other way around!)

In the current case, you're preferring new containers to old containers, and if they're never marked in the absent host tracker, you're just going to be creating containers and then abandoning them

👍🏼 make sense, will swap em!

mdaudali · 2022-10-31T11:32:32Z

...b-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/pool/CassandraService.java

    }

-    public void returnOrCreatePool(CassandraServer server, Optional<CassandraClientPoolingContainer> container) {


internal code search tool says there's no use, so we should be good :)

mdaudali · 2022-11-01T10:28:17Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+                        serversToAdd,
+                        allContainers,
+                        HumanReadableDuration.seconds(5),
+                        HumanReadableDuration.minutes(1));


Note that this will change when you switch from HumanReadableDuration to Duration in the other PR

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

mdaudali · 2022-11-01T10:46:47Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

            cassandra.refreshTokenRangesAndGetServers();
        }

+        Preconditions.checkState(
+                !(getCurrentPools().isEmpty() && !serversToAdd.isEmpty()),


Consider the equivalent via de Morgan's law: !gCP.isEmpty() || serversToAdd.isEmpty(), which has fewer negations and imo easier to read

mdaudali · 2022-11-01T11:07:10Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java


        setCassandraServersTo(CASS_SERVER_1, CASS_SERVER_2, CASS_SERVER_3);
        deterministicExecutor.tick(POOL_REFRESH_INTERVAL_SECONDS, TimeUnit.SECONDS);
-        assertThat(poolServers).containsExactlyInAnyOrder(CASS_SERVER_1, CASS_SERVER_2, CASS_SERVER_3);
+        assertThat(poolServers.keySet()).containsExactlyInAnyOrder(CASS_SERVER_1, CASS_SERVER_3);
+        assertThat(clientPool.getAbsentHostTracker().returnPool(CASS_SERVER_2)).isPresent();


consider making this a separate test, or changing the name to show that non-valid hosts are added to the absent host tracker

mdaudali · 2022-11-01T11:07:53Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        assertThat(poolServers.keySet()).containsExactlyInAnyOrder(CASS_SERVER_1, CASS_SERVER_2);
+
+        setCassandraServersTo(CASS_SERVER_1, CASS_SERVER_2, CASS_SERVER_3);
+        deterministicExecutor.tick(POOL_REFRESH_INTERVAL_SECONDS, TimeUnit.SECONDS);


consider making a method called refreshPool in the test that just ticks by pool refresh intervals seconds and calling that everywhere

mdaudali · 2022-11-01T11:11:00Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+
+    @Test
+    public void reuseContainerIfPreviouslyInvalid() {
+        setupThriftServers(ImmutableSet.of(CASS_SERVER_1.proxy(), CASS_SERVER_2.proxy()));


This test and the above are a bit hard to read, and I'm not entirely sure I understand what property one is testing that the other isn't.

If we can't stick to arrange act assert given the complexity of the tests, either simplify or add comments s.t. the state of the world is clear.

Also: add a test for https://github.com/palantir/atlasdb/pull/6317/files#diff-c8381f15121edbde8d40e9854428f137b054f9b222b0887bd590efefd98ccf4bR380

Let's remove reuseContainerIfPreviouslyInvalid, as it's effectively a subtest of reuseAbsentHostsContainerIfPresent.

reuseContainerIfPreviouslyInvalid primarily tested that we would use the container in the absent host tracker if it was present, where as reuseContainerIfPreviouslyInvalid tested that we actually store the container we generated!

mdaudali · 2022-11-01T11:13:35Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+    }
+
+    @Test
+    public void refreshHandlesNothingToAddOrRemove() {


what is the expected state for "handling"? Perhaps noServersAddedOrRemovedWhenCassandraServerListStatic or something like that.

mdaudali · 2022-11-01T11:14:03Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

@@ -108,25 +112,26 @@ public void setup() {
        when(runtimeConfig.unresponsiveHostBackoffTimeSeconds()).thenReturn(UNRESPONSIVE_HOST_BACKOFF_SECONDS);


aside, I'm surprised there are no tests to remove for the token ring consistency validation that we're removing...

changelog-app · 2022-11-01T17:25:58Z

Generate changelog in `changelog/@unreleased`

Type

Description

Removes token ring validation checks and instead checks for user-led split brain by ensuring any host added to our connection pool has its topology validated.

Check the box to generate changelog(s)

Generate changelog entry

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

mdaudali

looking good! A few nits and things to cleanup (and some notes left for posterity since they vanished on the force push). Should be small, then I can take another look tomorrow with you or send it off for second pass :)

mdaudali · 2022-11-02T19:55:54Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

    private DeterministicScheduler deterministicExecutor = new DeterministicScheduler();
-    private Set<CassandraServer> poolServers = new HashSet<>();
+    private Map<CassandraServer, CassandraClientPoolingContainer> poolServers = new HashMap<>();


I'm pretty sure this can be final, along with Cassandra and CassandraTopologyValidator

mdaudali · 2022-11-02T19:59:26Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

        when(config.socketTimeoutMillis()).thenReturn(1);
+        setupHostsWithInconsistentTopology();


This is a bit trippy - to me, this indicates that we're adding hosts with inconsistent topology, but actually, since its varargs, we're setting that there are no hosts with inconsistent topology.
a) I mentioned in a previous comment that if you're going to set empty, either overload the method or just have a method with a clearer name
b) This kinda messes with other mocks on this object (but since you're retrying and thus calling the method a second time, it works).

(a) yup makes sense, overloaded!
(b) removed, although fwiw it shouldn't be retrying at all, since we're mocking setting the return of getNewHostsWithInconsistentTopologiesAndRetry -- IIRC it's totally valid to overwrite an object already mocked?

For b) Turns out I was wrong.- I thought

when(config.autoRefreshNodes()).thenReturn(true); when(config.autoRefreshNodes()).thenReturn(false);

was identical to when(config.autoRefreshNodes()).thenReturn(true).thenReturn(false);
but a test tells me I'm wrong :/

mdaudali · 2022-11-02T20:00:18Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

@@ -299,7 +308,7 @@ public void hostIsAutomaticallyRemovedOnStartup() {
        setCassandraServersTo(CASS_SERVER_1);

        createClientPool();
-        assertThat(poolServers).containsExactlyInAnyOrder(CASS_SERVER_1);
+        assertThatMap(poolServers).containsOnlyKeys(CASS_SERVER_1);


Did not realise that there was a map specific method! you can just use assertThat though (since assertThat generates a MapAssert)

oh yeah duh

mdaudali · 2022-11-02T20:11:41Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        setCassandraServersTo(CASS_SERVER_1, CASS_SERVER_2, CASS_SERVER_3);
+        refreshPool();
+        assertThatMap(poolServers).containsOnlyKeys(CASS_SERVER_1, CASS_SERVER_3);
+        assertThat(absentHostTracker.returnPool(CASS_SERVER_2)).isPresent();


Since you ended up going for the separate test, I'd just remove this line (otherwise the next test is still redundant!)

mdaudali · 2022-11-02T20:12:06Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        setCassandraServersTo(CASS_SERVER_1, CASS_SERVER_2, CASS_SERVER_3);
+        setupHostsWithInconsistentTopology(CASS_SERVER_2);
+        createClientPool();
+        assertThat(absentHostTracker.returnPool(CASS_SERVER_2)).isPresent();


Lets add a whitespace just above this so it's clearer what the setup is and what we're asserting

mdaudali · 2022-11-02T20:15:07Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        createClientPool();
+        assertThatMap(poolServers).containsOnlyKeys(CASS_SERVER_1, CASS_SERVER_2);
+
+        setCassandraServersTo(CASS_SERVER_1, CASS_SERVER_2);


nit: I don't personally think this line adds much (at least, it seems to me if you don't call set cassandra servers, the list is static). This is a minor point if you feel it helps readability.

let's remove, my aim here was for readability, but as you said yourself you don't think it adds much :)

mdaudali · 2022-11-02T20:15:25Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+    }
+
+    @Test
+    public void throwsWhenTryingToAddServerPresentInCurerntServers() {


Suggested change

public void throwsWhenTryingToAddServerPresentInCurerntServers() {

public void throwsWhenTryingToAddServerPresentInCurrentServers() {

Although, I don't quite see why adding a server that's already present is a problem - shouldn't that be a no-op?

If it can be, then we can also remove the visible for testing on the validate method (yay!)

Yeah sorry should have left a comment for you explaining this in a bit more detail.

Basically, it should be impossible for us to have conflicting maps, due to the nature of how we determine serversToAdd (i.e. servers not present in the pool presently). If this sometime in the future throws, we then have strong evidence of a bug, and suggests something sketchy going on.

That said your other comment here has a valid point (which I initially went with), which was it shouldn't really matter, so arguably we shouldn't break in production on this. However, this means we are OK with potentially introducing this as a bug in the future, which is not ideal.

I don't feel strongly either way, but opt'd for this mode just b/c this really shouldn't happen, and could introduce a nasty bug later on if we change assumptions of topology validation.

mdaudali · 2022-11-02T20:19:27Z

versions.lock

@@ -89,11 +89,11 @@ com.palantir.dialogue:dialogue-core:3.69.0 (3 constraints: 883d5fe3)
 com.palantir.dialogue:dialogue-futures:3.69.0 (3 constraints: dd33d7c0)
 com.palantir.dialogue:dialogue-serde:3.69.0 (3 constraints: c62e7ca5)
 com.palantir.dialogue:dialogue-target:3.69.0 (7 constraints: a776356a)
-com.palantir.docker.compose:docker-compose-rule-core:1.8.0 (3 constraints: d92f0e11)
+com.palantir.docker.compose:docker-compose-rule-core:1.8.0 (3 constraints: dc2f1711)


noting to self that we should merge the other PR #6349 (if no one gets to it tomorrow, I'll do it) and then merge here so this isn't in the diff

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

mdaudali · 2022-11-02T20:42:25Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+                server -> absentHostTracker.trackAbsentCassandraServer(server, serversToAddContainers.get(server)));
+        if (!newHostsWithDifferingTopology.isEmpty()) {
+            log.warn(
+                    "Found hosts with differing topologies, or are potentially unreachable.",


Suggested change

"Found hosts with differing topologies, or are potentially unreachable.",

"Some hosts are potentially unreachable or have topologies that are not in consensus with the majority (if startup) or the current servers in the pool (if refreshing).",

mdaudali · 2022-11-03T10:19:21Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

@@ -106,18 +101,24 @@ public void shutdown() {

    private static final SafeLogger log = SafeLoggerFactory.get(CassandraClientPoolImpl.class);

+    private static final String EMPTY_CLIENT_POOL_MESSAGE =


What I think you were trying to do is make this public and use it in the test where we you do LoggableExceptionThrownBy. However, that's not the case (it's just used in the one precondition). As I mentioned in the MTD PR, we generally inline the message both here and the test. You could feasibly get away with just testing that the message contains No servers were successfully added to the pool, since the test is just to assert that it's our exception that we're throwin

mdaudali · 2022-11-03T10:21:05Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        when(config.autoRefreshNodes()).thenReturn(true);
+        setCassandraServersTo(CASS_SERVER_1);
+        assertThatLoggableExceptionThrownBy(this::createClientPool)
+                .isInstanceOf(SafeIllegalStateException.class)


nit: Let's also assert on the message :)

mdaudali · 2022-11-03T10:22:50Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

+        CassandraClientPoolImpl pool = createClientPool();
+        assertThat(poolServers).containsOnlyKeys(CASS_SERVER_1, CASS_SERVER_2);
+        assertThatThrownBy(() -> pool.validateNewHostsTopologiesAndMaybeAddToPool(poolServers, Set.of(CASS_SERVER_1)))
+                .isInstanceOf(IllegalArgumentException.class)


consider: This is the exception thrown by the immutable map (IIRC) - since it's not a safe log, we won't see the message nor the args.

Consider doing a precondition check to see if there are any duplicates and explicitly safe arg (since they're servers) so there's more debugging information

mdaudali · 2022-11-03T10:23:29Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

@@ -556,4 +658,18 @@ private void setupThriftServers(Set<InetSocketAddress> servers) {
                        .map(CassandraServer::of)
                        .collect(ImmutableSet.toImmutableSet()));
    }
+
+    private void setupHostsWithConsistentTopology() {
+        // Not hosts have inconsistent topology... therefore they're consistent!


mdaudali

lgtm, one nit but will not block, sending over to someone else to do a final pass

mdaudali · 2022-11-03T15:03:07Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+        Preconditions.checkArgument(
+                Sets.intersection(currentContainers.keySet(), serversToAddContainers.keySet())
+                        .isEmpty(),
+                "The current pool of servers should not have any server(s) that are being added.",


minor nit: this is declaring what should be the correct argument, as opposed to what the state is when this exception is thrown (i.e., closer to something like Tried adding a server that already exists. This indicates a bug in.... )

Jolyon-S

Need to review: tests, and new token range check. Thus far just reviewed the changes to the current cassandra code.

Jolyon-S · 2022-11-03T15:13:51Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+        Set<CassandraServer> validatedServersToAdd =
+                validateNewHostsTopologiesAndMaybeAddToPool(getCurrentPools(), serversToAdd);


Why are we re-fetching the current pools? We could just get this once, then derive currentServers from that. I think this is mainly nice as it should protect us a little more from any sneaky race conditions

Yes I agree, and there is a commit with this change, but it revealed a nasty bug: we must call getCurrentPools() again as we've modified the underlying pool (i.e. we removed the servers which are no longer used/present).

Alternatively, we could just filter these hosts from our "snapshot"

OK, I've just re-read the code, and the refresh codepath should be only ever run on a single thread at any one time. That means we should be resilient to any race conditions in this case.

Jolyon-S · 2022-11-03T15:15:19Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+                "No servers were successfully added to the pool. This means we could not come to a consensus on"
+                    + " cluster topology, and the client cannot start as there are no valid hosts. This state should"
+                    + " be transient (<5 minutes), and if it is not, indicates that the user may have accidentally"
+                    + " configured AltasDB to use two separate Cassandra clusters (i.e., user-led split brain).",


Can this happen after startup? This refresh task will happen by default every 2 minutes, and due to runtime config, possibly(?) also after any runtime config changes. As such, this error message might want to change to reflect that (what happens if you fail to refresh your hosts but you've already started up?)

yup makes sense, changed start to connect, although I suspect the case that this happens live to be extremely rare.

Jolyon-S · 2022-11-03T15:16:23Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+
+    /**
+     * Validates new servers to add to the cassandra client container pool,
+     * by checking them with the @see {@link com.palantir.atlasdb.keyvalue.cassandra.CassandraTopologyValidator}.


incorrect use of @see - I suggest you just remove it, the @link does the job.

Jolyon-S · 2022-11-03T15:18:15Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+                    + " cluster topology, and the client cannot start as there are no valid hosts. This state should"
+                    + " be transient (<5 minutes), and if it is not, indicates that the user may have accidentally"
+                    + " configured AltasDB to use two separate Cassandra clusters (i.e., user-led split brain).",
+                SafeArg.of("serversToAdd", serversToAdd));


Consider checking out CassandraLogHelper.collectionOfHosts

yeah lets chuck that in there

Jolyon-S · 2022-11-03T15:18:32Z

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java

+                SafeArg.of("serversToAdd", serversToAddContainers.keySet()),
+                SafeArg.of("currentServers", currentContainers.keySet()));


same here w.r.t. CassandraLogHelper

Jolyon-S

LGTM - one nit, but don't let that block you for another review round.

Jolyon-S · 2022-11-03T16:25:28Z

...cassandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolTest.java

@@ -110,22 +119,21 @@ public void setup() {
        when(config.getKeyspaceOrThrow()).thenReturn("ks");
        blacklist = new Blacklist(config, Refreshable.only(UNRESPONSIVE_HOST_BACKOFF_SECONDS));

-        doAnswer(invocation -> poolServers.add(getInvocationAddress(invocation)))
+        doAnswer(invocation -> poolServers.putIfAbsent(
+                        getCassandraServerFromInvocation(invocation), mock(CassandraClientPoolingContainer.class)))


you might want to extract out a single mock'd CCPC for ease of use here (unless you do some very fancy thing mocking individual CCPCs)

(unless you do some very fancy thing mocking individual CCPCs) we do need this unfortunately to tell if we are re-using containers or not :(

…cassandra/CassandraClientPoolImpl.java Co-authored-by: Mohammed Daudali <[email protected]>

svc-autorelease · 2022-11-04T12:16:21Z

Released 0.750.0

Sam-Kramer commented Oct 25, 2022

View reviewed changes

Sam-Kramer marked this pull request as ready for review October 25, 2022 16:57

Sam-Kramer added the do not merge label Oct 25, 2022

mdaudali reviewed Nov 1, 2022

View reviewed changes

Sam-Kramer changed the base branch from skramer/add-cluster-topology-check to develop November 1, 2022 17:25

Sam-Kramer force-pushed the skramer/remove-token-range-verify branch from d8bf1ff to 6829dbe Compare November 1, 2022 17:35

Sam-Kramer commented Nov 2, 2022

View reviewed changes

...cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraClientPoolImpl.java Outdated Show resolved Hide resolved

mdaudali reviewed Nov 2, 2022

View reviewed changes

mdaudali reviewed Nov 3, 2022

View reviewed changes

Sam-Kramer force-pushed the skramer/remove-token-range-verify branch from e4b0d8d to 8c43272 Compare November 3, 2022 14:58

mdaudali reviewed Nov 3, 2022

View reviewed changes

Jolyon-S reviewed Nov 3, 2022

View reviewed changes

Jolyon-S approved these changes Nov 3, 2022

View reviewed changes

Sam Kramer and others added 14 commits November 3, 2022 20:11

Remove token range checks, validate new servers on their topology

d885e0f

Fix integration tests

0c5e888

Add tests around tracking invalidated hosts

85e0f8e

Throw when pool is empty but servers have been added

0b5a35a

Better messages so we get paged less?

cdc9109

Add comment as to why timeout is 1 minute

8b6b3d0

Clean up tests to be pretty

933f7cd

Remove map utils and just use immutable map

134fb7f

Update atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/…

18e0b5f

…cassandra/CassandraClientPoolImpl.java Co-authored-by: Mohammed Daudali <[email protected]>

nits nits nits

fbb09f0

Describe transient in log message

d211b81

Use durations instead of human readable

a3adca4

Fix integ tests due to old version of map

e2549db

fix log msg

ca70588

Sam Kramer and others added 8 commits November 3, 2022 20:11

spotless

b581231

address nits

890c9d2

nits nits nits

3bf2e43

check the keyset of the map we are merging

a32eaa5

nits/spotless

e299505

better throw msg

ea82afa

Autorelease 0.749.0-rc1

e6a7364

rate limit warn for missing get_host_ids endpoint

bafb707

Sam-Kramer force-pushed the skramer/remove-token-range-verify branch from 7126d32 to bafb707 Compare November 3, 2022 20:11

only log when hosts have hard/soft failures

8523fe2

Sam-Kramer added autorelease merge when ready and removed do not merge labels Nov 4, 2022

Add generated changelog entries

675f00a

bulldozer-bot bot merged commit f404e45 into develop Nov 4, 2022

bulldozer-bot bot deleted the skramer/remove-token-range-verify branch November 4, 2022 12:16


		private MapUtils() {}

		public static <K, V> Map<K, V> combineMaps(Map<K, V> primary, Map<K, V> secondary) {

		}

		public void returnOrCreatePool(CassandraServer server, Optional<CassandraClientPoolingContainer> container) {

		@@ -108,25 +112,26 @@ public void setup() {
		when(runtimeConfig.unresponsiveHostBackoffTimeSeconds()).thenReturn(UNRESPONSIVE_HOST_BACKOFF_SECONDS);

		when(config.socketTimeoutMillis()).thenReturn(1);
		setupHostsWithInconsistentTopology();

	public void throwsWhenTryingToAddServerPresentInCurerntServers() {
	public void throwsWhenTryingToAddServerPresentInCurrentServers() {

	"Found hosts with differing topologies, or are potentially unreachable.",
	"Some hosts are potentially unreachable or have topologies that are not in consensus with the majority (if startup) or the current servers in the pool (if refreshing).",

		@@ -106,18 +101,24 @@ public void shutdown() {

		private static final SafeLogger log = SafeLoggerFactory.get(CassandraClientPoolImpl.class);

		private static final String EMPTY_CLIENT_POOL_MESSAGE =

		Set<CassandraServer> validatedServersToAdd =
		validateNewHostsTopologiesAndMaybeAddToPool(getCurrentPools(), serversToAdd);

		SafeArg.of("serversToAdd", serversToAddContainers.keySet()),
		SafeArg.of("currentServers", currentContainers.keySet()));

Remove token range checks, validate new servers on their topology #6317

Remove token range checks, validate new servers on their topology #6317

Conversation

Sam-Kramer commented Oct 25, 2022 • edited Loading

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

Sam-Kramer Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

mdaudali Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdaudali Oct 31, 2022 • 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

mdaudali left a comment • 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

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

changelog-app bot commented Nov 1, 2022 • edited by Sam-Kramer Loading

Generate changelog in changelog/@unreleased

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

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

Jolyon-S 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

Choose a reason for hiding this comment

Jolyon-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sam-Kramer commented Oct 25, 2022 •

edited

Loading

Sam-Kramer Oct 25, 2022 •

edited

Loading

mdaudali Oct 31, 2022 •

edited

Loading

mdaudali Oct 31, 2022 •

edited

Loading

mdaudali left a comment •

edited

Loading

changelog-app bot commented Nov 1, 2022 •

edited by Sam-Kramer

Loading

Generate changelog in `changelog/@unreleased`