Optimize CassandraService node selection #6074

schlosna · 2022-06-07T02:27:04Z

Goals (and why):
The com.palantir.atlasdb.keyvalue.cassandra.pool.CassandraService.getRandomHostByActiveConnections(Set) method is used heavily by AtlasDB Cassandra read path when identifying which Cassandra nodes to target for requests. Given how hot this path is, there were many unnecessary ImmutableMap.copyOf(currentPools).

==COMMIT_MSG==
Optimize CassandraService node selection

Explicitly use ImmutableSet and SetView to identify snapshots and views of cluster state
both for clarity and to avoid excessive intermediate collection copies on hot code paths.
==COMMIT_MSG==

Implementation Description (bullets):

Iterate over the ConcurrentHashMap<CassandraServer, CassandraClientPoolingContainer> currentPools to construct an ImmutableMap snapshot of (maybe local) Cassandra nodes.
Local nodes is always an ImmutableSet snapshot view of cluster state.
Enforces the cluster snapshot added in Fix WeightedHosts concurrency bug #5266
Removes ImmutableSet copy when filtering blocked hosts.

Testing (What was existing testing like? What have you done to improve it?):

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):

Blocked hosts are filtered without requiring set copy.

changelog-app · 2022-06-07T02:27:08Z

Generate changelog in `changelog/@unreleased`

Type

Description

Remove ImmutableMap copy from CassandraService.getRandomHostByActiveConnections(Set)

Check the box to generate changelog(s)

Generate changelog entry

schlosna · 2022-06-07T02:38:35Z

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

@@ -108,8 +106,8 @@ private boolean isHostHealthy(CassandraClientPoolingContainer container) {
        }
    }

-    public Set<CassandraServer> filterBlacklistedHostsFrom(Collection<CassandraServer> potentialHosts) {
-        return Sets.difference(ImmutableSet.copyOf(potentialHosts), blacklist.keySet());


We should be able to make the argument an ImmutableSet, no? The only annoying one is in the use of getRandomGoodHostForPredicate - the Sets.filter should be an immutable result (since the input is immutable - at least, I assume that's how that works!) but I think we'd need to use another copyOf to get the right type. Alternatively, if you reverted that back to a stream and used ImmutableSet.toImmutableSet() we'd have the right types everywhere.

I went back and forth if we wanted ImmutableSet in the public method args/return type, and could go either way. One of my intents with marking more of the private method returns with ImmutableSet and SetView was to make it easier to reason about what it a point in time snapshot, and what may be lazily computed view.

schlosna · 2022-06-07T02:40:00Z

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

    private final Map<CassandraServer, CassandraClientPoolingContainer> currentPools = new ConcurrentHashMap<>();
    private volatile Map<CassandraServer, String> hostToDatacenter = ImmutableMap.of();

    private List<CassandraServer> cassandraHosts;

-    private volatile Set<CassandraServer> localHosts = ImmutableSet.of();
+    private volatile ImmutableSet<CassandraServer> localHosts = ImmutableSet.of();


ensure we're always reasoning about an immutable snapshot copy of local Cassandra hosts that is refreshing asynchronously

schlosna · 2022-06-07T02:43:39Z

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

        Set<CassandraServer> localFilteredHosts = maybeFilterLocalHosts(desiredHosts);
-
-        Map<CassandraServer, CassandraClientPoolingContainer> matchingPools = KeyedStream.stream(
-                        ImmutableMap.copyOf(currentPools))


schlosna · 2022-06-07T19:31:20Z

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

-                        ImmutableMap.copyOf(currentPools))
-                .filterKeys(localFilteredHosts::contains)
-                .collectToMap();
+        ImmutableMap<CassandraServer, CassandraClientPoolingContainer> matchingPools = currentPools.entrySet().stream()


this will still allocate a java.util.concurrent.ConcurrentHashMap.EntrySpliterator, but we'll only iterate once instead of twice

are we concerned about concurrent modification of the currentPools though? My understanding is that the previous implementation mitigated this by making a copy immediately, but now that we don't do that, the entrySet can change midway through?

Although I suppose the concern about this matchingPools map changing after construction (which was the original motivator behind the copyOf) is still safe as we collect to an immutable map.

we have to iterate the currentPools once to either create the immediate snapshot map copy or make the filtered map copy, so I opted to to the one shot iterate & filter which would be more up-to-date in the sense of reflecting any concurrent additions/removals and avoids some intermediate overhead, and once we've constructed that snapshot of local cluster state, that's what we use to select the desired Cassandra target node.

I agree that that sounds fine, but I'm curious to know what happens to a stream if the underlying collection changes under you: that's my main concern (i.e. will it throw, etc.). That notwithstanding, this is fine

A stream or iterator on ConcurrentHashMap.entrySet will provide a weakly consistent view of the underlying concurrent map, so we will see all entries as they exist when stream is created and may see updates, but will not throw on concurrent modifications.

Per https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#entrySet-- & https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html#Weakly

Most concurrent Collection implementations (including most Queues) also differ from the usual java.util conventions in that their Iterators and Spliterators provide weakly consistent rather than fast-fail traversal:

they may proceed concurrently with other operations

they will never throw ConcurrentModificationException

they are guaranteed to traverse elements as they existed upon construction exactly once, and may (but are not guaranteed to) reflect any modifications subsequent to construction.

fabulous - thanks for clearing that up

…leSet snapshots

Jolyon-S

Thanks for the PR - I have a few questions / suggestions.

Jolyon-S · 2022-06-08T10:12:07Z

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

@@ -108,8 +106,8 @@ private boolean isHostHealthy(CassandraClientPoolingContainer container) {
        }
    }

-    public Set<CassandraServer> filterBlacklistedHostsFrom(Collection<CassandraServer> potentialHosts) {
-        return Sets.difference(ImmutableSet.copyOf(potentialHosts), blacklist.keySet());


We should be able to make the argument an ImmutableSet, no? The only annoying one is in the use of getRandomGoodHostForPredicate - the Sets.filter should be an immutable result (since the input is immutable - at least, I assume that's how that works!) but I think we'd need to use another copyOf to get the right type. Alternatively, if you reverted that back to a stream and used ImmutableSet.toImmutableSet() we'd have the right types everywhere.

Jolyon-S · 2022-06-08T10:15:50Z

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

+    void setServersInPoolTo(ImmutableSet<CassandraServer> desiredServers) {
+        ImmutableSet<CassandraServer> cachedServers = getCachedServers();
+        ImmutableSet<CassandraServer> serversToAdd =
+                ImmutableSet.copyOf(Sets.difference(desiredServers, cachedServers));


it's a shame there isn't a nice method for Sets.difference that returns an immutable set if both inputs are also immutable

Now that this is a lazy view of 2 immutable sets, we could get rid of the copy.

Jolyon-S · 2022-06-08T10:18:20Z

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

+        return verifierConfig.servers().accept(ThriftHostsExtractingVisitor.INSTANCE).stream()
+                .anyMatch(host -> attemptToCreateIfNotExists(host, verifierConfig));


no aversion whatsoever to this inlining, but is this a performance improvement or just a driveby? I'd be surprised if it was not the latter

Yeah, this was a drive by in-line because the switch to visitor singleton enum made this wrap, so just inlined it

Jolyon-S · 2022-06-08T10:22:17Z

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

@@ -180,7 +182,7 @@ public Set<CassandraServer> refreshTokenRangesAndGetServers() {
            tokenMap = tokensInterner.intern(newTokenRing.build());
            logHostToDatacenterMapping(hostToDatacentersThisRefresh);
            hostToDatacenter = hostToDatacentersThisRefresh;
-            return servers;
+            return ImmutableSet.copyOf(servers);


could we make servers an immutable set builder instead? I assume that'd be no worse

Jolyon-S · 2022-06-08T10:26:13Z

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

-    private Set<InetSocketAddress> getServersSocketAddressesFromConfig() {
-        return runtimeConfig.get().servers().accept(new ThriftHostsExtractingVisitor());
+    private ImmutableSet<InetSocketAddress> getServersSocketAddressesFromConfig() {
+        return ImmutableSet.copyOf(runtimeConfig.get().servers().accept(ThriftHostsExtractingVisitor.INSTANCE));


I see that ImmutableSet.copyOf won't necessarily copy the elements if the underlying set is already immutable (which it will be in this instance, as the visitor is extracting elements from an Immutables object, which uses ImmutableSet under the hood). This does still do a cast and instanceof check; ideally we'd be able to change that so the return type is ImmutableSet, but this is probably still acceptable as-is.

Similar to comment above, this change was getting quite large without refactoring public method return types, but happy to adjust those as well

Jolyon-S · 2022-06-08T10:36:04Z

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

    }

-    private void incrementAbsenceCountIfPresent(CassandraServer cassandraServer) {
+    private synchronized void incrementAbsenceCountIfPresent(CassandraServer cassandraServer) {


good catch - I think this was deemed safe because it is only accessed by a single thread, but agree that guaranteeing this by adding synchronised is appropriate.

...ssandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/pool/CassandraServiceTest.java

Jolyon-S · 2022-06-08T10:40:25Z

...ssandra/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/pool/CassandraServiceTest.java

@@ -245,6 +245,26 @@ public void selectsFromAllHostsIfDatacenterMappingNotAvailable() {
        assertThat(suggestedHosts).containsExactlyInAnyOrderElementsOf(allHosts);
    }

+    @Test
+    public void getRandomHostByActiveConnections() {


nit:

Suggested change

public void getRandomHostByActiveConnections() {

public void getRandomHostByActiveConnectionsReturnsDesiredHost() {

Jolyon-S · 2022-06-08T10:45:10Z

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

-                        ImmutableMap.copyOf(currentPools))
-                .filterKeys(localFilteredHosts::contains)
-                .collectToMap();
+        ImmutableMap<CassandraServer, CassandraClientPoolingContainer> matchingPools = currentPools.entrySet().stream()


Although I suppose the concern about this matchingPools map changing after construction (which was the original motivator behind the copyOf) is still safe as we collect to an immutable map.

Jolyon-S · 2022-06-08T10:46:02Z

changelog/@unreleased/pr-6074.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Remove ImmutableMap copy from CassandraService.getRandomHostByActiveConnections(Set)


Unless I misunderstand, this PR has a fair bit more in scope now.

will update

Jolyon-S

Looks good - the only thing left was a clarification on what happens if the collection is modified under your stream (and a suggested nit), but otherwise LGTM and doesn't need to block on further approval to merge!

Follow up to #6074 to further improve the Cassandra pooling to avoid unnecessary overhead for services that are coordinating many nodes. We can avoid intermediate collections when validating the Cassandra destination ports for a given pool.

Optimize CassandraService port checks Follow up to #6074 to further improve the Cassandra pooling to avoid unnecessary overhead for services that are coordinating many nodes. We can avoid intermediate collections when validating the Cassandra destination ports for a given pool.

schlosna added 6 commits June 6, 2022 21:32

CassandraService volatile local hosts is ImmutableSet snapshot

d59edd5

CassandraService.getRandomHostByActiveConnections is testable

a6f6d4c

CassandraService.getRandomHostByActiveConnections immutable snapshot

ae29ff0

fixup test

2e97590

WeightedServers enforces Cassandra cluster state snapshot

ff91ccc

CassandraService uses range map of tokens to immutable set of nodes

cd752dc

Blocked hosts are filtered without requiring set copy.

Add generated changelog entries

a83f995

schlosna commented Jun 7, 2022

View reviewed changes

schlosna requested a review from jeremyk-91 June 7, 2022 21:06

schlosna added 4 commits June 7, 2022 21:04

CassandraService always uses ImmutableRangeMap view of cluster

0301fa7

ThriftHostsExtractingVisitor is singleton enum instance

64dab73

CassandraAbsentHostTracker ensures proper synchronization and Immutab…

8fa7811

…leSet snapshots

CassandraService uses ImmutableSet and Sets.SetView

1fa062c

Jolyon-S reviewed Jun 8, 2022

View reviewed changes

schlosna added 3 commits June 8, 2022 13:21

ThriftHostsExtractingVisitor returns ImmutableSet

6e88245

Explicit ImmutableSet method arguments

007e733

Address CassandraServiceTest feedback

46ff7c4

schlosna changed the title ~~Remove ImmutableMap copy from CassandraService.getRandomHostByActiveConnections(Set)~~ Optimize CassandraService node selection Jun 8, 2022

svc-changelog and others added 2 commits June 8, 2022 20:11

Add generated changelog entries

2da2c1e

Update changelog

45aad23

Jolyon-S self-requested a review June 9, 2022 12:18

Jolyon-S approved these changes Jun 9, 2022

View reviewed changes

schlosna added the merge when ready label Jun 9, 2022

bulldozer-bot bot merged commit 2af4918 into develop Jun 9, 2022

bulldozer-bot bot deleted the ds/weighted-cass branch June 9, 2022 13:44

This was referenced Aug 25, 2022

Optimize CassandraService port checks #6198

Merged

Deduplicate Cassandra service nodes #6207

Merged

schlosna mentioned this pull request Sep 8, 2022

Refactor to improve CassandaraService test coverage #6222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize CassandraService node selection #6074

Optimize CassandraService node selection #6074

schlosna commented Jun 7, 2022 •

edited

Loading

changelog-app bot commented Jun 7, 2022 •

edited by schlosna

Loading

schlosna Jun 7, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

schlosna Jun 7, 2022

schlosna Jun 7, 2022 •

edited

Loading

schlosna Jun 7, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022 •

edited

Loading

Jolyon-S Jun 9, 2022

schlosna Jun 9, 2022

Jolyon-S Jun 9, 2022

Jolyon-S left a comment

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S left a comment

		return verifierConfig.servers().accept(ThriftHostsExtractingVisitor.INSTANCE).stream()
		.anyMatch(host -> attemptToCreateIfNotExists(host, verifierConfig));

	public void getRandomHostByActiveConnections() {
	public void getRandomHostByActiveConnectionsReturnsDesiredHost() {

Optimize CassandraService node selection #6074

Optimize CassandraService node selection #6074

Conversation

schlosna commented Jun 7, 2022 • edited Loading

changelog-app bot commented Jun 7, 2022 • edited by schlosna Loading

Generate changelog in changelog/@unreleased

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

schlosna Jun 7, 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

schlosna Jun 8, 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

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

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

schlosna commented Jun 7, 2022 •

edited

Loading

changelog-app bot commented Jun 7, 2022 •

edited by schlosna

Loading

Generate changelog in `changelog/@unreleased`

schlosna Jun 7, 2022 •

edited

Loading

schlosna Jun 8, 2022 •

edited

Loading