From 6c1ca051550edb3ffe72434850b4c56feeb06bd4 Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Fri, 7 Sep 2018 19:28:16 +0100 Subject: [PATCH 1/8] introduce ReplicaList and ReplicaMap inner classes --- .../org/apache/cassandra/db/ReadCommand.java | 15 +- .../locator/AbstractReplicaCollection.java | 351 ++++++++++++++++-- .../apache/cassandra/locator/Endpoints.java | 24 +- .../cassandra/locator/EndpointsForRange.java | 31 +- .../cassandra/locator/EndpointsForToken.java | 32 +- .../cassandra/locator/RangesAtEndpoint.java | 56 ++- .../cassandra/locator/ReplicaCollection.java | 11 + .../cassandra/locator/ReplicaLayout.java | 1 + .../cassandra/locator/ReplicaPlans.java | 7 +- .../cassandra/locator/SimpleStrategy.java | 7 +- .../service/reads/AbstractReadExecutor.java | 14 +- .../locator/ReplicaCollectionTest.java | 19 +- 12 files changed, 440 insertions(+), 128 deletions(-) diff --git a/src/java/org/apache/cassandra/db/ReadCommand.java b/src/java/org/apache/cassandra/db/ReadCommand.java index ada4ae65d843..cbef7ea97b7c 100644 --- a/src/java/org/apache/cassandra/db/ReadCommand.java +++ b/src/java/org/apache/cassandra/db/ReadCommand.java @@ -66,6 +66,9 @@ import org.apache.cassandra.utils.FBUtilities; import org.apache.cassandra.utils.HashingUtils; +import static com.google.common.collect.Iterables.any; +import static com.google.common.collect.Iterables.filter; + /** * General interface for storage-engine read commands (common to both range and * single partition commands). @@ -326,10 +329,10 @@ public ReadCommand copyAsTransientQuery(Replica replica) /** * Returns a copy of this command with acceptsTransient set to true. */ - public ReadCommand copyAsTransientQuery(ReplicaCollection replicas) + public ReadCommand copyAsTransientQuery(Iterable replicas) { - if (Iterables.any(replicas, Replica::isFull)) - throw new IllegalArgumentException("Can't make a transient request on full replicas: " + replicas.filter(Replica::isFull)); + if (any(replicas, Replica::isFull)) + throw new IllegalArgumentException("Can't make a transient request on full replicas: " + Iterables.toString(filter(replicas, Replica::isFull))); return copyAsTransientQuery(); } @@ -348,10 +351,10 @@ public ReadCommand copyAsDigestQuery(Replica replica) /** * Returns a copy of this command with isDigestQuery set to true. */ - public ReadCommand copyAsDigestQuery(ReplicaCollection replicas) + public ReadCommand copyAsDigestQuery(Iterable replicas) { - if (Iterables.any(replicas, Replica::isTransient)) - throw new IllegalArgumentException("Can't make a digest request on a transient replica " + replicas.filter(Replica::isTransient)); + if (any(replicas, Replica::isTransient)) + throw new IllegalArgumentException("Can't make a digest request on a transient replica " + Iterables.toString(filter(replicas, Replica::isTransient))); return copyAsDigestQuery(); } diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index 94ff991aa5a2..b2fdb842fe66 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -18,12 +18,16 @@ package org.apache.cassandra.locator; +import com.carrotsearch.hppc.ObjectIntOpenHashMap; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Iterables; -import java.util.ArrayList; +import java.util.AbstractMap; +import java.util.AbstractSet; +import java.util.Arrays; +import java.util.Collection; import java.util.Comparator; import java.util.Iterator; -import java.util.List; import java.util.Objects; import java.util.Set; import java.util.function.BiConsumer; @@ -41,7 +45,7 @@ */ public abstract class AbstractReplicaCollection> implements ReplicaCollection { - protected static final List EMPTY_LIST = new ArrayList<>(); // since immutable, can safely return this to avoid megamorphic callsites + protected static final ReplicaList EMPTY_LIST = new ReplicaList(); // since immutable, can safely return this to avoid megamorphic callsites public static , B extends Builder> Collector collector(Set characteristics, Supplier supplier) { @@ -58,16 +62,305 @@ public abstract class AbstractReplicaCollection list; - protected final boolean isSnapshot; - protected AbstractReplicaCollection(List list, boolean isSnapshot) + /** + * A simple list with no comodification checks and immutability by default (only append permitted, and only one initial copy) + * this permits us to reduce the amount of garbage generated, by not wrapping iterators or unnecessarily copying + * and reduces the amount of indirection necessary, as well as ensuring monomorphic callsites + */ + protected static class ReplicaList implements Iterable + { + private static final Replica[] EMPTY = new Replica[0]; + Replica[] contents; + int begin, size; + + public ReplicaList() { this(0); } + public ReplicaList(int capacity) { contents = capacity == 0 ? EMPTY : new Replica[capacity]; } + public ReplicaList(Replica[] contents, int begin, int size) { this.contents = contents; this.begin = begin; this.size = size; } + + public boolean isSubList(ReplicaList subList) + { + return subList.contents == contents; + } + + public Replica get(int index) + { + if (index > size) + throw new IndexOutOfBoundsException(); + return contents[begin + index]; + } + + public void add(Replica replica) + { + // can only add to full array - if we have sliced it, we must be a snapshot + assert begin == 0; + if (size == contents.length) + { + int newSize; + if (size < 3) newSize = 3; + else if (size < 9) newSize = 9; + else newSize = size * 2; + contents = Arrays.copyOf(contents, newSize); + } + contents[size++] = replica; + } + + public int size() + { + return size; + } + + public boolean isEmpty() + { + return size == 0; + } + + public ReplicaList subList(int begin, int end) + { + if (end > size || begin > end) throw new IndexOutOfBoundsException(); + return new ReplicaList(contents, this.begin + begin, end - begin); + } + + public ReplicaList sorted(Comparator comparator) + { + Replica[] copy = Arrays.copyOfRange(contents, begin, begin + size); + Arrays.sort(copy, comparator); + return new ReplicaList(copy, 0, copy.length); + } + + public Stream stream() + { + return Arrays.stream(contents, begin, begin + size); + } + + @Override + public Iterator iterator() + { + return new Iterator() + { + final int end = begin + size; + int i = begin; + @Override + public boolean hasNext() + { + return i < end; + } + + @Override + public Replica next() + { + return contents[i++]; + } + }; + } + + public Iterator transformIterator(Function function) + { + return new Iterator() + { + final int end = begin + size; + int i = begin; + @Override + public boolean hasNext() + { + return i < end; + } + + @Override + public K next() + { + return function.apply(contents[i++]); + } + }; + } + + private Iterator filterIterator(Predicate predicate, int limit) + { + return new Iterator() + { + final int end = begin + size; + int next = begin; + int count = 0; + { updateNext(); } + void updateNext() + { + if (count == limit) next = end; + while (next < end && !predicate.test(contents[next])) + ++next; + ++count; + } + @Override + public boolean hasNext() + { + return next < end; + } + + @Override + public Replica next() + { + if (!hasNext()) throw new IllegalStateException(); + Replica result = contents[next++]; + updateNext(); + return result; + } + }; + } + + @VisibleForTesting + public boolean equals(Object to) + { + if (to == null || to.getClass() != ReplicaList.class) + return false; + ReplicaList that = (ReplicaList) to; + if (this.size != that.size) return false; + for (int i = 0 ; i < size ; ++i) + if (!this.get(i).equals(that.get(i))) + return false; + return true; + } + } + + /** + * A simple map that ensures the underlying list's iteration order is maintained, and can be shared with + * subLists (either produced via subList, or via filter that naturally produced a subList). + * This permits us to reduce the amount of garbage generated, by not unnecessarily copying, + * reduces the amount of indirection necessary, as well as ensuring monomorphic callsites. + * The underlying map is also more efficient, particularly for such small collections as we typically produce. + */ + protected static class ReplicaMap extends AbstractMap + { + private final Function toKey; + private final ReplicaList list; + private final ObjectIntOpenHashMap map; + private Set keySet; + private Set> entrySet; + + abstract class AbstractImmutableSet extends AbstractSet + { + @Override + public boolean removeAll(Collection c) { throw new UnsupportedOperationException(); } + @Override + public boolean remove(Object o) { throw new UnsupportedOperationException(); } + @Override + public int size() { return list.size(); } + } + + class KeySet extends AbstractImmutableSet + { + @Override + public boolean contains(Object o) { return containsKey(o); } + @Override + public Iterator iterator() { return list.transformIterator(toKey); } + } + + class EntrySet extends AbstractImmutableSet> + { + @Override + public boolean contains(Object o) + { + if (!(o instanceof Entry)) return false; + return Objects.equals(get(((Entry) o).getKey()), ((Entry) o).getValue()); + } + + @Override + public Iterator> iterator() + { + return list.transformIterator(r -> new SimpleImmutableEntry<>(toKey.apply(r), r)); + } + } + + private static boolean putIfAbsent(ObjectIntOpenHashMap map, Function toKey, Replica replica, int index) + { + K key = toKey.apply(replica); + int otherIndex = map.put(key, index + 1); + if (otherIndex == 0) + return true; + map.put(key, otherIndex); + return false; + } + + public ReplicaMap(ReplicaList list, Function toKey) + { + // 8*0.65 => RF=5; 16*0.65 ==> RF=10 + // use list capacity if empty, otherwise use actual list size + ObjectIntOpenHashMap map = new ObjectIntOpenHashMap<>(list.size == 0 ? list.contents.length : list.size, 0.65f); + for (int i = list.begin ; i < list.begin + list.size ; ++i) + { + boolean inserted = putIfAbsent(map, toKey, list.contents[i], i); + assert inserted; + } + this.toKey = toKey; + this.list = list; + this.map = map; + } + public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHashMap map) + { + this.toKey = toKey; + this.list = list; + this.map = map; + } + + boolean internalPutIfAbsent(Replica replica, int index) { return putIfAbsent(map, toKey, replica, index); } + + @Override + public boolean containsKey(Object key) + { + return get((K)key) != null; + } + + public Replica get(Object key) + { + int index = map.get((K)key) - 1; + if (index < list.begin || index >= list.begin + list.size) + return null; + return list.contents[index]; + } + + @Override + public Replica remove(Object key) + { + throw new UnsupportedOperationException(); + } + + @Override + public Set keySet() + { + Set ks = keySet; + if (ks == null) + keySet = ks = new KeySet(); + return ks; + } + + @Override + public Set> entrySet() + { + Set> es = entrySet; + if (es == null) + entrySet = es = new EntrySet(); + return es; + } + + public int size() + { + return list.size(); + } + + public ReplicaMap subList(ReplicaList subList) + { + assert subList.contents == list.contents; + return new ReplicaMap<>(subList, toKey, map); + } + } + + protected final ReplicaList list; + final boolean isSnapshot; + AbstractReplicaCollection(ReplicaList list, boolean isSnapshot) { this.list = list; this.isSnapshot = isSnapshot; } // if subList == null, should return self (or a clone thereof) - protected abstract C snapshot(List subList); + protected abstract C snapshot(ReplicaList newList); protected abstract C self(); /** * construct a new Mutable of our own type, so that we can concatenate @@ -79,24 +372,18 @@ public C snapshot() { return isSnapshot ? self() : snapshot(list.isEmpty() ? EMPTY_LIST - : new ArrayList<>(list)); + : list); } /** see {@link ReplicaCollection#subList(int, int)}*/ public final C subList(int start, int end) { - List subList; - if (isSnapshot) - { - if (start == 0 && end == size()) return self(); - else if (start == end) subList = EMPTY_LIST; - else subList = list.subList(start, end); - } - else - { - if (start == end) subList = EMPTY_LIST; - else subList = new ArrayList<>(list.subList(start, end)); // TODO: we could take a subList here, but comodification checks stop us - } + if (isSnapshot && start == 0 && end == size()) return self(); + + ReplicaList subList; + if (start == end) subList = EMPTY_LIST; + else subList = list.subList(start, end); + return snapshot(subList); } @@ -122,7 +409,7 @@ public final C filter(Predicate predicate, int limit) if (isEmpty()) return snapshot(); - List copy = null; + ReplicaList copy = null; int beginRun = -1, endRun = -1; int i = 0; for (; i < list.size() ; ++i) @@ -136,7 +423,7 @@ else if (beginRun < 0) beginRun = i; else if (endRun > 0) { - copy = new ArrayList<>(Math.min(limit, (list.size() - i) + (endRun - beginRun))); + copy = new ReplicaList(Math.min(limit, (list.size() - i) + (endRun - beginRun))); for (int j = beginRun ; j < endRun ; ++j) copy.add(list.get(j)); copy.add(list.get(i)); @@ -160,12 +447,22 @@ else if (beginRun >= 0 && endRun < 0) return snapshot(copy); } + /** see {@link ReplicaCollection#filterLazily(Predicate)}*/ + public final Iterable filterLazily(Predicate predicate) + { + return filterLazily(predicate, Integer.MAX_VALUE); + } + + /** see {@link ReplicaCollection#filterLazily(Predicate,int)}*/ + public final Iterable filterLazily(Predicate predicate, int limit) + { + return () -> list.filterIterator(predicate, limit); + } + /** see {@link ReplicaCollection#sorted(Comparator)}*/ - public final C sorted(Comparator comparator) + public final C sorted(Comparator comparator) { - List copy = new ArrayList<>(list); - copy.sort(comparator); - return snapshot(copy); + return snapshot(list.sorted(comparator)); } public final Replica get(int i) @@ -224,7 +521,7 @@ public final int hashCode() @Override public final String toString() { - return list.toString(); + return Iterables.toString(list); } static > C concat(C replicas, C extraReplicas, Mutable.Conflict ignoreConflicts) diff --git a/src/java/org/apache/cassandra/locator/Endpoints.java b/src/java/org/apache/cassandra/locator/Endpoints.java index 28e578cda56a..c5cedfef7e15 100644 --- a/src/java/org/apache/cassandra/locator/Endpoints.java +++ b/src/java/org/apache/cassandra/locator/Endpoints.java @@ -36,11 +36,12 @@ */ public abstract class Endpoints> extends AbstractReplicaCollection { - static final Map EMPTY_MAP = Collections.unmodifiableMap(new LinkedHashMap<>()); + static final ReplicaMap endpointMap(ReplicaList list) { return new ReplicaMap<>(list, Replica::endpoint); } + static final ReplicaMap EMPTY_MAP = endpointMap(EMPTY_LIST); - volatile Map byEndpoint; + ReplicaMap byEndpoint; - Endpoints(List list, boolean isSnapshot, Map byEndpoint) + Endpoints(ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) { super(list, isSnapshot); this.byEndpoint = byEndpoint; @@ -54,9 +55,9 @@ public Set endpoints() public Map byEndpoint() { - Map map = byEndpoint; + ReplicaMap map = byEndpoint; if (map == null) - byEndpoint = map = buildByEndpoint(list); + byEndpoint = map = endpointMap(list); return map; } @@ -75,19 +76,6 @@ public boolean contains(Replica replica) replica); } - private static Map buildByEndpoint(List list) - { - // TODO: implement a delegating map that uses our superclass' list, and is immutable - Map byEndpoint = new LinkedHashMap<>(list.size()); - for (Replica replica : list) - { - Replica prev = byEndpoint.put(replica.endpoint(), replica); - assert prev == null : "duplicate endpoint in EndpointsForRange: " + prev + " and " + replica; - } - - return Collections.unmodifiableMap(byEndpoint); - } - public E withoutSelf() { InetAddressAndPort self = FBUtilities.getBroadcastAddressAndPort(); diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index f812951ccd3a..e790abd3f599 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -40,11 +40,11 @@ public class EndpointsForRange extends Endpoints { private final Range range; - private EndpointsForRange(Range range, List list, boolean isSnapshot) + private EndpointsForRange(Range range, ReplicaList list, boolean isSnapshot) { this(range, list, isSnapshot, null); } - private EndpointsForRange(Range range, List list, boolean isSnapshot, Map byEndpoint) + private EndpointsForRange(Range range, ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) { super(list, isSnapshot, byEndpoint); this.range = range; @@ -76,17 +76,21 @@ public EndpointsForRange self() } @Override - protected EndpointsForRange snapshot(List snapshot) + protected EndpointsForRange snapshot(ReplicaList newList) { - if (snapshot.isEmpty()) return empty(range); - return new EndpointsForRange(range, snapshot, true); + if (newList.isEmpty()) return empty(range); + ReplicaMap byEndpoint = null; + if (this.byEndpoint != null && list.isSubList(newList)) + byEndpoint = this.byEndpoint.subList(newList); + return new EndpointsForRange(range, newList, true, byEndpoint); } public static class Mutable extends EndpointsForRange implements ReplicaCollection.Mutable { boolean hasSnapshot; public Mutable(Range range) { this(range, 0); } - public Mutable(Range range, int capacity) { super(range, new ArrayList<>(capacity), false, new LinkedHashMap<>(capacity)); } + public Mutable(Range range, int capacity) { this(range, new ReplicaList(capacity)); } + private Mutable(Range range, ReplicaList list) { super(range, list, false, endpointMap(list)); } public void add(Replica replica, Conflict ignoreConflict) { @@ -95,17 +99,16 @@ public void add(Replica replica, Conflict ignoreConflict) if (!replica.range().contains(super.range)) throw new IllegalArgumentException("Replica " + replica + " does not contain " + super.range); - Replica prev = super.byEndpoint.put(replica.endpoint(), replica); - if (prev != null) + if (!super.byEndpoint.internalPutIfAbsent(replica, list.size())) { - super.byEndpoint.put(replica.endpoint(), prev); // restore prev switch (ignoreConflict) { case DUPLICATE: - if (prev.equals(replica)) + if (byEndpoint().get(replica.endpoint()).equals(replica)) break; case NONE: - throw new IllegalArgumentException("Conflicting replica added (expected unique endpoints): " + replica + "; existing: " + prev); + throw new IllegalArgumentException("Conflicting replica added (expected unique endpoints): " + + replica + "; existing: " + byEndpoint().get(replica.endpoint())); case ALL: } return; @@ -124,7 +127,7 @@ public Map byEndpoint() private EndpointsForRange get(boolean isSnapshot) { - return new EndpointsForRange(super.range, super.list, isSnapshot, Collections.unmodifiableMap(super.byEndpoint)); + return new EndpointsForRange(super.range, super.list, isSnapshot, super.byEndpoint); } public EndpointsForRange asImmutableView() @@ -166,10 +169,10 @@ public static EndpointsForRange empty(Range range) public static EndpointsForRange of(Replica replica) { // we only use ArrayList or ArrayList.SubList, to ensure callsites are bimorphic - ArrayList one = new ArrayList<>(1); + ReplicaList one = new ReplicaList(1); one.add(replica); // we can safely use singletonMap, as we only otherwise use LinkedHashMap - return new EndpointsForRange(replica.range(), one, true, Collections.unmodifiableMap(Collections.singletonMap(replica.endpoint(), replica))); + return new EndpointsForRange(replica.range(), one, true, endpointMap(one)); } public static EndpointsForRange of(Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index 3446dc9e8201..549e52ca280e 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -19,6 +19,7 @@ package org.apache.cassandra.locator; import com.google.common.base.Preconditions; +import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; import java.util.ArrayList; @@ -37,12 +38,12 @@ public class EndpointsForToken extends Endpoints { private final Token token; - private EndpointsForToken(Token token, List list, boolean isSnapshot) + private EndpointsForToken(Token token, ReplicaList list, boolean isSnapshot) { this(token, list, isSnapshot, null); } - EndpointsForToken(Token token, List list, boolean isSnapshot, Map byEndpoint) + EndpointsForToken(Token token, ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) { super(list, isSnapshot, byEndpoint); this.token = token; @@ -67,17 +68,21 @@ public EndpointsForToken self() } @Override - protected EndpointsForToken snapshot(List subList) + protected EndpointsForToken snapshot(ReplicaList newList) { - if (subList.isEmpty()) return empty(token); - return new EndpointsForToken(token, subList, true); + if (newList.isEmpty()) return empty(token); + ReplicaMap byEndpoint = null; + if (this.byEndpoint != null && list.isSubList(newList)) + byEndpoint = this.byEndpoint.subList(newList); + return new EndpointsForToken(token, newList, true, byEndpoint); } public static class Mutable extends EndpointsForToken implements ReplicaCollection.Mutable { boolean hasSnapshot; public Mutable(Token token) { this(token, 0); } - public Mutable(Token token, int capacity) { super(token, new ArrayList<>(capacity), false, new LinkedHashMap<>(capacity)); } + public Mutable(Token token, int capacity) { this(token, new ReplicaList(capacity)); } + private Mutable(Token token, ReplicaList list) { super(token, list, false, endpointMap(list)); } public void add(Replica replica, Conflict ignoreConflict) { @@ -86,17 +91,16 @@ public void add(Replica replica, Conflict ignoreConflict) if (!replica.range().contains(super.token)) throw new IllegalArgumentException("Replica " + replica + " does not contain " + super.token); - Replica prev = super.byEndpoint.put(replica.endpoint(), replica); - if (prev != null) + if (!super.byEndpoint.internalPutIfAbsent(replica, list.size())) { - super.byEndpoint.put(replica.endpoint(), prev); // restore prev switch (ignoreConflict) { case DUPLICATE: - if (prev.equals(replica)) + if (byEndpoint().get(replica.endpoint()).equals(replica)) break; case NONE: - throw new IllegalArgumentException("Conflicting replica added (expected unique endpoints): " + replica + "; existing: " + prev); + throw new IllegalArgumentException("Conflicting replica added (expected unique endpoints): " + + replica + "; existing: " + byEndpoint().get(replica.endpoint())); case ALL: } return; @@ -115,7 +119,7 @@ public Map byEndpoint() private EndpointsForToken get(boolean isSnapshot) { - return new EndpointsForToken(super.token, super.list, isSnapshot, Collections.unmodifiableMap(super.byEndpoint)); + return new EndpointsForToken(super.token, super.list, isSnapshot, super.byEndpoint); } public EndpointsForToken asImmutableView() @@ -153,10 +157,10 @@ public static EndpointsForToken empty(Token token) public static EndpointsForToken of(Token token, Replica replica) { // we only use ArrayList or ArrayList.SubList, to ensure callsites are bimorphic - ArrayList one = new ArrayList<>(1); + ReplicaList one = new ReplicaList(1); one.add(replica); // we can safely use singletonMap, as we only otherwise use LinkedHashMap - return new EndpointsForToken(token, one, true, Collections.unmodifiableMap(Collections.singletonMap(replica.endpoint(), replica))); + return new EndpointsForToken(token, one, true, endpointMap(one)); } public static EndpointsForToken of(Token token, Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index 8319d92c2759..03e7abccd96f 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -25,17 +25,14 @@ import org.apache.cassandra.dht.Token; import java.net.UnknownHostException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.stream.Collector; -import java.util.stream.Collectors; import static com.google.common.collect.Iterables.all; import static org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict.*; @@ -46,18 +43,19 @@ */ public class RangesAtEndpoint extends AbstractReplicaCollection { - private static final Map, Replica> EMPTY_MAP = Collections.unmodifiableMap(new LinkedHashMap<>()); + private static ReplicaMap> rangeMap(ReplicaList list) { return new ReplicaMap<>(list, Replica::range); } + private static final ReplicaMap> EMPTY_MAP = rangeMap(EMPTY_LIST); private final InetAddressAndPort endpoint; - private volatile Map, Replica> byRange; + private ReplicaMap> byRange; private volatile RangesAtEndpoint fullRanges; private volatile RangesAtEndpoint transRanges; - private RangesAtEndpoint(InetAddressAndPort endpoint, List list, boolean isSnapshot) + private RangesAtEndpoint(InetAddressAndPort endpoint, ReplicaList list, boolean isSnapshot) { this(endpoint, list, isSnapshot, null); } - private RangesAtEndpoint(InetAddressAndPort endpoint, List list, boolean isSnapshot, Map, Replica> byRange) + private RangesAtEndpoint(InetAddressAndPort endpoint, ReplicaList list, boolean isSnapshot, ReplicaMap> byRange) { super(list, isSnapshot); this.endpoint = endpoint; @@ -86,17 +84,20 @@ public Set> ranges() public Map, Replica> byRange() { - Map, Replica> map = byRange; + ReplicaMap> map = byRange; if (map == null) - byRange = map = buildByRange(list); + byRange = map = rangeMap(list); return map; } @Override - protected RangesAtEndpoint snapshot(List subList) + protected RangesAtEndpoint snapshot(ReplicaList newList) { - if (subList.isEmpty()) return empty(endpoint); - return new RangesAtEndpoint(endpoint, subList, true); + if (newList.isEmpty()) return empty(endpoint); + ReplicaMap> byRange = null; + if (this.byRange != null && list.isSubList(newList)) + byRange = this.byRange.subList(newList); + return new RangesAtEndpoint(endpoint, newList, true, byRange); } @Override @@ -152,19 +153,6 @@ public boolean contains(Range range, boolean isFull) return replica != null && replica.isFull() == isFull; } - private static Map, Replica> buildByRange(List list) - { - // TODO: implement a delegating map that uses our superclass' list, and is immutable - Map, Replica> byRange = new LinkedHashMap<>(list.size()); - for (Replica replica : list) - { - Replica prev = byRange.put(replica.range(), replica); - assert prev == null : "duplicate range in RangesAtEndpoint: " + prev + " and " + replica; - } - - return Collections.unmodifiableMap(byRange); - } - /** * @return if there are no wrap around ranges contained in this RangesAtEndpoint, return self; * otherwise, return a RangesAtEndpoint covering the same logical portions of the ring, but with those ranges unwrapped @@ -205,7 +193,8 @@ public static class Mutable extends RangesAtEndpoint implements ReplicaCollectio { boolean hasSnapshot; public Mutable(InetAddressAndPort endpoint) { this(endpoint, 0); } - public Mutable(InetAddressAndPort endpoint, int capacity) { super(endpoint, new ArrayList<>(capacity), false, new LinkedHashMap<>(capacity)); } + public Mutable(InetAddressAndPort endpoint, int capacity) { this(endpoint, new ReplicaList(capacity)); } + private Mutable(InetAddressAndPort endpoint, ReplicaList list) { super(endpoint, list, false, rangeMap(list)); } public void add(Replica replica, Conflict ignoreConflict) { @@ -214,17 +203,16 @@ public void add(Replica replica, Conflict ignoreConflict) if (!Objects.equals(super.endpoint, replica.endpoint())) throw new IllegalArgumentException("Replica " + replica + " has incorrect endpoint (expected " + super.endpoint + ")"); - Replica prev = super.byRange.put(replica.range(), replica); - if (prev != null) + if (!super.byRange.internalPutIfAbsent(replica, list.size())) { - super.byRange.put(replica.range(), prev); // restore prev switch (ignoreConflict) { case DUPLICATE: - if (prev.equals(replica)) + if (byRange().get(replica.range()).equals(replica)) break; case NONE: - throw new IllegalArgumentException("Conflicting replica added (expected unique ranges): " + replica + "; existing: " + prev); + throw new IllegalArgumentException("Conflicting replica added (expected unique ranges): " + + replica + "; existing: " + byRange().get(replica.range())); case ALL: } return; @@ -243,7 +231,7 @@ public Map, Replica> byRange() public RangesAtEndpoint get(boolean isSnapshot) { - return new RangesAtEndpoint(super.endpoint, super.list, isSnapshot, Collections.unmodifiableMap(super.byRange)); + return new RangesAtEndpoint(super.endpoint, super.list, isSnapshot, super.byRange); } public RangesAtEndpoint asImmutableView() @@ -281,9 +269,9 @@ public static RangesAtEndpoint empty(InetAddressAndPort endpoint) public static RangesAtEndpoint of(Replica replica) { - ArrayList one = new ArrayList<>(1); + ReplicaList one = new ReplicaList(1); one.add(replica); - return new RangesAtEndpoint(replica.endpoint(), one, true, Collections.unmodifiableMap(Collections.singletonMap(replica.range(), replica))); + return new RangesAtEndpoint(replica.endpoint(), one, true, rangeMap(one)); } public static RangesAtEndpoint of(Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/ReplicaCollection.java b/src/java/org/apache/cassandra/locator/ReplicaCollection.java index d1006dc49212..b658ba4ce746 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/ReplicaCollection.java @@ -81,6 +81,17 @@ public interface ReplicaCollection> extends Itera */ public abstract C filter(Predicate predicate, int maxSize); + /** + * @return a *lazily constructed* Iterable over this collection, containing the Replica that match the provided predicate. + */ + public abstract Iterable filterLazily(Predicate predicate); + + /** + * @return a *lazily constructed* Iterable over this collection, containing the Replica that match the provided predicate. + * Only the first maxSize matching items will be returned. + */ + public abstract Iterable filterLazily(Predicate predicate, int maxSize); + /** * @return an *eagerly constructed* copy of this collection containing the Replica at positions [start..end); * An effort will be made to either return ourself, or a subList, where possible. diff --git a/src/java/org/apache/cassandra/locator/ReplicaLayout.java b/src/java/org/apache/cassandra/locator/ReplicaLayout.java index f48c989721c2..cba4f6898805 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaLayout.java +++ b/src/java/org/apache/cassandra/locator/ReplicaLayout.java @@ -193,6 +193,7 @@ public interface ForToken */ public static ReplicaLayout.ForTokenWrite forTokenWriteLiveAndDown(Keyspace keyspace, Token token) { + // TODO: these should be cached, not the natural replicas // TODO: race condition to fetch these. implications?? EndpointsForToken natural = keyspace.getReplicationStrategy().getNaturalReplicasForToken(token); EndpointsForToken pending = StorageService.instance.getTokenMetadata().pendingEndpointsForToken(token, keyspace.getName()); diff --git a/src/java/org/apache/cassandra/locator/ReplicaPlans.java b/src/java/org/apache/cassandra/locator/ReplicaPlans.java index 87f3c093a97c..ed4c5413411e 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaPlans.java +++ b/src/java/org/apache/cassandra/locator/ReplicaPlans.java @@ -57,8 +57,6 @@ import java.util.function.Predicate; import static com.google.common.collect.Iterables.any; -import static com.google.common.collect.Iterables.filter; -import static com.google.common.collect.Iterables.limit; import static org.apache.cassandra.db.ConsistencyLevel.EACH_QUORUM; import static org.apache.cassandra.db.ConsistencyLevel.eachQuorumFor; import static org.apache.cassandra.db.ConsistencyLevel.localQuorumFor; @@ -170,7 +168,6 @@ static void assureSufficientLiveReplicas(Keyspace keyspace, ConsistencyLevel con } } - /** * Construct a ReplicaPlan for writing to exactly one node, with CL.ONE. This node is *assumed* to be alive. */ @@ -391,14 +388,14 @@ E select(Keyspace keyspace, ConsistencyLevel consistencyLevel, L liveAndDown, L assert consistencyLevel != EACH_QUORUM; ReplicaCollection.Mutable contacts = liveAndDown.all().newMutable(liveAndDown.all().size()); - contacts.addAll(filter(liveAndDown.natural(), Replica::isFull)); + contacts.addAll(liveAndDown.natural().filterLazily(Replica::isFull)); contacts.addAll(liveAndDown.pending()); // TODO: this doesn't correctly handle LOCAL_QUORUM (or EACH_QUORUM at all) int liveCount = contacts.count(live.all()::contains); int requiredTransientCount = consistencyLevel.blockForWrite(keyspace, liveAndDown.pending()) - liveCount; if (requiredTransientCount > 0) - contacts.addAll(limit(filter(live.natural(), Replica::isTransient), requiredTransientCount)); + contacts.addAll(live.natural().filterLazily(Replica::isTransient, requiredTransientCount)); return contacts.asSnapshot(); } }; diff --git a/src/java/org/apache/cassandra/locator/SimpleStrategy.java b/src/java/org/apache/cassandra/locator/SimpleStrategy.java index 7a000b77bf1b..d924ac5682fd 100644 --- a/src/java/org/apache/cassandra/locator/SimpleStrategy.java +++ b/src/java/org/apache/cassandra/locator/SimpleStrategy.java @@ -20,9 +20,11 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Collection; +import java.util.Comparator; import java.util.Iterator; import java.util.Map; +import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.dht.Range; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.dht.Token; @@ -65,7 +67,10 @@ public EndpointsForRange calculateNaturalReplicas(Token token, TokenMetadata met if (!replicas.containsEndpoint(ep)) replicas.add(new Replica(ep, replicaRange, replicas.size() < rf.fullReplicas)); } - return replicas.build(); + + // group endpoints by DC, so that we can cheaply filter them to a given DC + IEndpointSnitch snitch = DatabaseDescriptor.getEndpointSnitch(); + return replicas.build().sorted(Comparator.comparing(r -> snitch.getDatacenter(r.endpoint()))); } public ReplicationFactor getReplicationFactor() diff --git a/src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java b/src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java index 8d0f14c8d470..b1307a1a6724 100644 --- a/src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java +++ b/src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java @@ -117,26 +117,26 @@ protected void makeFullDataRequests(ReplicaCollection replicas) makeRequests(command, replicas); } - protected void makeTransientDataRequests(ReplicaCollection replicas) + protected void makeTransientDataRequests(Iterable replicas) { makeRequests(command.copyAsTransientQuery(replicas), replicas); } - protected void makeDigestRequests(ReplicaCollection replicas) + protected void makeDigestRequests(Iterable replicas) { assert all(replicas, Replica::isFull); // only send digest requests to full replicas, send data requests instead to the transient replicas makeRequests(command.copyAsDigestQuery(replicas), replicas); } - private void makeRequests(ReadCommand readCommand, ReplicaCollection replicas) + private void makeRequests(ReadCommand readCommand, Iterable replicas) { boolean hasLocalEndpoint = false; - Preconditions.checkArgument(replicas.stream().allMatch(replica -> replica.isFull() || !readCommand.isDigestQuery()), - "Can not send digest requests to transient replicas"); for (Replica replica: replicas) { + assert replica.isFull() || readCommand.acceptsTransient(); + InetAddressAndPort endpoint = replica.endpoint(); if (replica.isSelf()) { @@ -172,8 +172,8 @@ public void executeAsync() EndpointsForToken selected = replicaPlan().contacts(); EndpointsForToken fullDataRequests = selected.filter(Replica::isFull, initialDataRequestCount); makeFullDataRequests(fullDataRequests); - makeTransientDataRequests(selected.filter(Replica::isTransient)); - makeDigestRequests(selected.filter(r -> r.isFull() && !fullDataRequests.contains(r))); + makeTransientDataRequests(selected.filterLazily(Replica::isTransient)); + makeDigestRequests(selected.filterLazily(r -> r.isFull() && !fullDataRequests.contains(r))); } /** diff --git a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java index c289d50e33be..3dce1413fc9a 100644 --- a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java +++ b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java @@ -154,13 +154,23 @@ private void assertSubList(C subCollection, int from, int to) } else { - List subList = this.test.list.subList(from, to); + AbstractReplicaCollection.ReplicaList subList = this.test.list.subList(from, to); if (test.isSnapshot) - Assert.assertSame(subList.getClass(), subCollection.list.getClass()); + Assert.assertSame(subList.contents, subCollection.list.contents); Assert.assertEquals(subList, subCollection.list); } } + private void assertSubSequence(Iterable subSequence, int from, int to) + { + AbstractReplicaCollection.ReplicaList subList = this.test.list.subList(from, to); + if (!elementsEqual(subList, subSequence)) + { + elementsEqual(subList, subSequence); + } + Assert.assertTrue(elementsEqual(subList, subSequence)); + } + void testSubList(int subListDepth, int filterDepth, int sortDepth) { if (test.isSnapshot) @@ -191,6 +201,8 @@ void testFilter(int subListDepth, int filterDepth, int sortDepth) Predicate removeFirst = r -> !r.equals(canonicalList.get(0)); assertSubList(test.filter(removeFirst), 1, canonicalList.size()); assertSubList(test.filter(removeFirst, 1), 1, Math.min(canonicalList.size(), 2)); + assertSubSequence(test.filterLazily(removeFirst), 1, canonicalList.size()); + assertSubSequence(test.filterLazily(removeFirst, 1), 1, Math.min(canonicalList.size(), 2)); } if (test.size() <= 1) @@ -202,6 +214,7 @@ void testFilter(int subListDepth, int filterDepth, int sortDepth) int last = canonicalList.size() - 1; Predicate removeLast = r -> !r.equals(canonicalList.get(last)); assertSubList(test.filter(removeLast), 0, last); + assertSubSequence(test.filterLazily(removeLast), 0, last); } if (test.size() <= 2) @@ -210,6 +223,8 @@ void testFilter(int subListDepth, int filterDepth, int sortDepth) Predicate removeMiddle = r -> !r.equals(canonicalList.get(canonicalList.size() / 2)); TestCase filtered = new TestCase<>(test.filter(removeMiddle), ImmutableList.copyOf(filter(canonicalList, removeMiddle::test))); filtered.testAll(subListDepth, filterDepth - 1, sortDepth); + Assert.assertTrue(elementsEqual(filtered.canonicalList, test.filterLazily(removeMiddle, Integer.MAX_VALUE))); + Assert.assertTrue(elementsEqual(limit(filter(canonicalList, removeMiddle::test), canonicalList.size() - 2), test.filterLazily(removeMiddle, canonicalList.size() - 2))); } void testCount() From fbdbd6795dca6980816e9ac58c430a21031696f5 Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Tue, 18 Sep 2018 13:50:48 +0100 Subject: [PATCH 2/8] cleanup RangesAtEndpoint transient/full cached filtering; use wherever possible --- .../cassandra/db/DiskBoundaryManager.java | 8 ++-- .../db/compaction/CompactionManager.java | 28 +++++++------- .../db/streaming/CassandraStreamManager.java | 2 +- .../apache/cassandra/locator/Endpoints.java | 2 + .../cassandra/locator/RangesAtEndpoint.java | 37 ++++++++----------- .../cassandra/service/StorageService.java | 2 +- .../db/compaction/AntiCompactionTest.java | 4 +- 7 files changed, 39 insertions(+), 44 deletions(-) diff --git a/src/java/org/apache/cassandra/db/DiskBoundaryManager.java b/src/java/org/apache/cassandra/db/DiskBoundaryManager.java index 0961a4275f56..69aabfd730b4 100644 --- a/src/java/org/apache/cassandra/db/DiskBoundaryManager.java +++ b/src/java/org/apache/cassandra/db/DiskBoundaryManager.java @@ -123,19 +123,19 @@ private static DiskBoundaries getDiskBoundaryValue(ColumnFamilyStore cfs) * * The final entry in the returned list will always be the partitioner maximum tokens upper key bound */ - private static List getDiskBoundaries(RangesAtEndpoint ranges, IPartitioner partitioner, Directories.DataDirectory[] dataDirectories) + private static List getDiskBoundaries(RangesAtEndpoint replicas, IPartitioner partitioner, Directories.DataDirectory[] dataDirectories) { assert partitioner.splitter().isPresent(); Splitter splitter = partitioner.splitter().get(); boolean dontSplitRanges = DatabaseDescriptor.getNumTokens() > 1; - List weightedRanges = new ArrayList<>(ranges.size()); + List weightedRanges = new ArrayList<>(replicas.size()); // note that Range.sort unwraps any wraparound ranges, so we need to sort them here - for (Range r : Range.sort(ranges.fullRanges())) + for (Range r : Range.sort(replicas.onlyFull().ranges())) weightedRanges.add(new Splitter.WeightedRange(1.0, r)); - for (Range r : Range.sort(ranges.transientRanges())) + for (Range r : Range.sort(replicas.onlyTransient().ranges())) weightedRanges.add(new Splitter.WeightedRange(0.1, r)); weightedRanges.sort(Comparator.comparing(Splitter.WeightedRange::left)); diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 39f3db513f2e..b8b4993a5aed 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -522,8 +522,8 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo // if local ranges is empty, it means no data should remain final RangesAtEndpoint replicas = StorageService.instance.getLocalReplicas(keyspace.getName()); final Set> allRanges = replicas.ranges(); - final Set> transientRanges = replicas.filter(Replica::isTransient).ranges(); - final Set> fullRanges = replicas.filter(Replica::isFull).ranges(); + final Set> transientRanges = replicas.onlyTransient().ranges(); + final Set> fullRanges = replicas.onlyFull().ranges(); final boolean hasIndexes = cfStore.indexManager.hasIndexes(); return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() @@ -722,14 +722,14 @@ private static void mutateFullyContainedSSTables(ColumnFamilyStore cfs, * Caller must reference the validatedForRepair sstables (via ParentRepairSession.getActiveRepairedSSTableRefs(..)). * * @param cfs - * @param ranges token ranges to be repaired + * @param replicas token ranges to be repaired * @param validatedForRepair SSTables containing the repaired ranges. Should be referenced before passing them. * @param sessionID the repair session we're anti-compacting for * @throws InterruptedException * @throws IOException */ public void performAnticompaction(ColumnFamilyStore cfs, - RangesAtEndpoint ranges, + RangesAtEndpoint replicas, Refs validatedForRepair, LifecycleTransaction txn, UUID sessionID) throws IOException @@ -738,21 +738,21 @@ public void performAnticompaction(ColumnFamilyStore cfs, { ActiveRepairService.ParentRepairSession prs = ActiveRepairService.instance.getParentRepairSession(sessionID); Preconditions.checkArgument(!prs.isPreview(), "Cannot anticompact for previews"); - Preconditions.checkArgument(!ranges.isEmpty(), "No ranges to anti-compact"); + Preconditions.checkArgument(!replicas.isEmpty(), "No ranges to anti-compact"); if (logger.isInfoEnabled()) logger.info("{} Starting anticompaction for {}.{} on {}/{} sstables", PreviewKind.NONE.logPrefix(sessionID), cfs.keyspace.getName(), cfs.getTableName(), validatedForRepair.size(), cfs.getLiveSSTables().size()); if (logger.isTraceEnabled()) - logger.trace("{} Starting anticompaction for ranges {}", PreviewKind.NONE.logPrefix(sessionID), ranges); + logger.trace("{} Starting anticompaction for ranges {}", PreviewKind.NONE.logPrefix(sessionID), replicas); Set sstables = new HashSet<>(validatedForRepair); - validateSSTableBoundsForAnticompaction(sessionID, sstables, ranges); - mutateFullyContainedSSTables(cfs, validatedForRepair, sstables.iterator(), ranges.fullRanges(), txn, sessionID, false); - mutateFullyContainedSSTables(cfs, validatedForRepair, sstables.iterator(), ranges.transientRanges(), txn, sessionID, true); + validateSSTableBoundsForAnticompaction(sessionID, sstables, replicas); + mutateFullyContainedSSTables(cfs, validatedForRepair, sstables.iterator(), replicas.onlyFull().ranges(), txn, sessionID, false); + mutateFullyContainedSSTables(cfs, validatedForRepair, sstables.iterator(), replicas.onlyTransient().ranges(), txn, sessionID, true); assert txn.originals().equals(sstables); if (!sstables.isEmpty()) - doAntiCompaction(cfs, ranges, txn, sessionID); + doAntiCompaction(cfs, replicas, txn, sessionID); txn.finish(); } finally @@ -963,8 +963,8 @@ public void forceUserDefinedCleanup(String dataFiles) Keyspace keyspace = cfs.keyspace; final RangesAtEndpoint replicas = StorageService.instance.getLocalReplicas(keyspace.getName()); final Set> allRanges = replicas.ranges(); - final Set> transientRanges = replicas.filter(Replica::isTransient).ranges(); - final Set> fullRanges = replicas.filter(Replica::isFull).ranges(); + final Set> transientRanges = replicas.onlyTransient().ranges(); + final Set> fullRanges = replicas.onlyFull().ranges(); boolean hasIndexes = cfs.indexManager.hasIndexes(); SSTableReader sstable = lookupSSTable(cfs, entry.getValue()); @@ -1512,8 +1512,8 @@ private int antiCompactGroup(ColumnFamilyStore cfs, transWriter.switchWriter(CompactionManager.createWriterForAntiCompaction(cfs, destination, expectedBloomFilterSize, UNREPAIRED_SSTABLE, pendingRepair, true, sstableAsSet, txn)); unrepairedWriter.switchWriter(CompactionManager.createWriterForAntiCompaction(cfs, destination, expectedBloomFilterSize, UNREPAIRED_SSTABLE, NO_PENDING_REPAIR, false, sstableAsSet, txn)); - Predicate fullChecker = !ranges.fullRanges().isEmpty() ? new Range.OrderedRangeContainmentChecker(ranges.fullRanges()) : t -> false; - Predicate transChecker = !ranges.transientRanges().isEmpty() ? new Range.OrderedRangeContainmentChecker(ranges.transientRanges()) : t -> false; + Predicate fullChecker = !ranges.onlyFull().isEmpty() ? new Range.OrderedRangeContainmentChecker(ranges.onlyFull().ranges()) : t -> false; + Predicate transChecker = !ranges.onlyTransient().isEmpty() ? new Range.OrderedRangeContainmentChecker(ranges.onlyTransient().ranges()) : t -> false; while (ci.hasNext()) { try (UnfilteredRowIterator partition = ci.next()) diff --git a/src/java/org/apache/cassandra/db/streaming/CassandraStreamManager.java b/src/java/org/apache/cassandra/db/streaming/CassandraStreamManager.java index 6c2631c82889..b88a5d66828f 100644 --- a/src/java/org/apache/cassandra/db/streaming/CassandraStreamManager.java +++ b/src/java/org/apache/cassandra/db/streaming/CassandraStreamManager.java @@ -142,7 +142,7 @@ else if (pendingRepair == ActiveRepairService.NO_PENDING_REPAIR) }).refs); - List> normalizedFullRanges = Range.normalize(replicas.filter(Replica::isFull).ranges()); + List> normalizedFullRanges = Range.normalize(replicas.onlyFull().ranges()); List> normalizedAllRanges = Range.normalize(replicas.ranges()); //Create outgoing file streams for ranges possibly skipping repaired ranges in sstables List streams = new ArrayList<>(refs.size()); diff --git a/src/java/org/apache/cassandra/locator/Endpoints.java b/src/java/org/apache/cassandra/locator/Endpoints.java index c5cedfef7e15..b27ef1d7eba3 100644 --- a/src/java/org/apache/cassandra/locator/Endpoints.java +++ b/src/java/org/apache/cassandra/locator/Endpoints.java @@ -39,6 +39,8 @@ public abstract class Endpoints> extends AbstractReplicaC static final ReplicaMap endpointMap(ReplicaList list) { return new ReplicaMap<>(list, Replica::endpoint); } static final ReplicaMap EMPTY_MAP = endpointMap(EMPTY_LIST); + // volatile not needed, as has only final members, + // besides (transitively) those that cache objects that themselves have only final members ReplicaMap byEndpoint; Endpoints(ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index 03e7abccd96f..f531a18ac8b0 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -47,9 +47,12 @@ public class RangesAtEndpoint extends AbstractReplicaCollection> EMPTY_MAP = rangeMap(EMPTY_LIST); private final InetAddressAndPort endpoint; + + // volatile not needed, as all of these caching collections have final members, + // besides (transitively) those that cache objects that themselves have only final members private ReplicaMap> byRange; - private volatile RangesAtEndpoint fullRanges; - private volatile RangesAtEndpoint transRanges; + private RangesAtEndpoint onlyFull; + private RangesAtEndpoint onlyTransient; private RangesAtEndpoint(InetAddressAndPort endpoint, ReplicaList list, boolean isSnapshot) { @@ -121,30 +124,20 @@ public boolean contains(Replica replica) replica); } - public RangesAtEndpoint full() - { - RangesAtEndpoint coll = fullRanges; - if (fullRanges == null) - fullRanges = coll = filter(Replica::isFull); - return coll; - } - - public RangesAtEndpoint trans() - { - RangesAtEndpoint coll = transRanges; - if (transRanges == null) - transRanges = coll = filter(Replica::isTransient); - return coll; - } - - public Collection> fullRanges() + public RangesAtEndpoint onlyFull() { - return full().ranges(); + RangesAtEndpoint result = onlyFull; + if (onlyFull == null) + onlyFull = result = filter(Replica::isFull); + return result; } - public Collection> transientRanges() + public RangesAtEndpoint onlyTransient() { - return trans().ranges(); + RangesAtEndpoint result = onlyTransient; + if (onlyTransient == null) + onlyTransient = result = filter(Replica::isTransient); + return result; } public boolean contains(Range range, boolean isFull) diff --git a/src/java/org/apache/cassandra/service/StorageService.java b/src/java/org/apache/cassandra/service/StorageService.java index caa732acb7f3..5536f173e201 100644 --- a/src/java/org/apache/cassandra/service/StorageService.java +++ b/src/java/org/apache/cassandra/service/StorageService.java @@ -3717,7 +3717,7 @@ else if (option.isInLocalDCOnly()) } else { - Iterables.addAll(option.getRanges(), getLocalReplicas(keyspace).filter(Replica::isFull).ranges()); + Iterables.addAll(option.getRanges(), getLocalReplicas(keyspace).onlyFull().ranges()); } } if (option.getRanges().isEmpty() || Keyspace.open(keyspace).getReplicationStrategy().getReplicationFactor().allReplicas < 2) diff --git a/test/unit/org/apache/cassandra/db/compaction/AntiCompactionTest.java b/test/unit/org/apache/cassandra/db/compaction/AntiCompactionTest.java index f514ea65d032..597879fc81c5 100644 --- a/test/unit/org/apache/cassandra/db/compaction/AntiCompactionTest.java +++ b/test/unit/org/apache/cassandra/db/compaction/AntiCompactionTest.java @@ -155,8 +155,8 @@ private SSTableStats antiCompactRanges(ColumnFamilyStore store, RangesAtEndpoint SSTableStats stats = new SSTableStats(); stats.numLiveSSTables = store.getLiveSSTables().size(); - Predicate fullContains = t -> Iterables.any(ranges.fullRanges(), r -> r.contains(t)); - Predicate transContains = t -> Iterables.any(ranges.transientRanges(), r -> r.contains(t)); + Predicate fullContains = t -> Iterables.any(ranges.onlyFull().ranges(), r -> r.contains(t)); + Predicate transContains = t -> Iterables.any(ranges.onlyTransient().ranges(), r -> r.contains(t)); for (SSTableReader sstable : store.getLiveSSTables()) { assertFalse(sstable.isRepaired()); From 0a02d94b550b6825e63476f260a32818d4d58453 Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Tue, 18 Sep 2018 13:53:57 +0100 Subject: [PATCH 3/8] circleci --- .circleci/config.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5a84f724fcf8..76a2c9f84178 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -58,16 +58,16 @@ with_dtest_jobs_only: &with_dtest_jobs_only - build # Set env_settings, env_vars, and workflows/build_and_run_tests based on environment env_settings: &env_settings - <<: *default_env_settings - #<<: *high_capacity_env_settings + #<<: *default_env_settings + <<: *high_capacity_env_settings env_vars: &env_vars - <<: *resource_constrained_env_vars - #<<: *high_capacity_env_vars + #<<: *resource_constrained_env_vars + <<: *high_capacity_env_vars workflows: version: 2 - build_and_run_tests: *default_jobs + #build_and_run_tests: *default_jobs #build_and_run_tests: *with_dtest_jobs_only - #build_and_run_tests: *with_dtest_jobs + build_and_run_tests: *with_dtest_jobs docker_image: &docker_image kjellman/cassandra-test:0.4.3 version: 2 jobs: From 78e5e6cbba2d9f7c9d1764173c78f1c350f7c8c1 Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Tue, 18 Sep 2018 17:22:58 +0100 Subject: [PATCH 4/8] small cleanup + comments --- .../cassandra/locator/AbstractReplicaCollection.java | 9 ++++++++- .../org/apache/cassandra/locator/EndpointsForRange.java | 5 +---- .../org/apache/cassandra/locator/EndpointsForToken.java | 6 +----- .../org/apache/cassandra/locator/RangesAtEndpoint.java | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index b2fdb842fe66..bc99414cd3f9 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -230,6 +230,9 @@ protected static class ReplicaMap extends AbstractMap { private final Function toKey; private final ReplicaList list; + // we maintain a map of key -> index in our list; this lets us share with subLists (or between Mutable and snapshots) + // since we only need to corroborate that the list index we find is within the bounds of our list + // (if not, it's a shared map, and the key only occurs in one of our ancestors) private final ObjectIntOpenHashMap map; private Set keySet; private Set> entrySet; @@ -292,6 +295,7 @@ public ReplicaMap(ReplicaList list, Function toKey) this.list = list; this.map = map; } + public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHashMap map) { this.toKey = toKey; @@ -299,6 +303,7 @@ public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHas this.map = map; } + // to be used only by subclasses of AbstractReplicaCollection boolean internalPutIfAbsent(Replica replica, int index) { return putIfAbsent(map, toKey, replica, index); } @Override @@ -310,6 +315,8 @@ public boolean containsKey(Object key) public Replica get(Object key) { int index = map.get((K)key) - 1; + // since this map can be shared between sublists (or snapshots of mutables) + // we have to first corroborate that the index we've found is actually within our list's bounds if (index < list.begin || index >= list.begin + list.size) return null; return list.contents[index]; @@ -344,7 +351,7 @@ public int size() return list.size(); } - public ReplicaMap subList(ReplicaList subList) + ReplicaMap isSubList(ReplicaList subList) { assert subList.contents == list.contents; return new ReplicaMap<>(subList, toKey, map); diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index e790abd3f599..2a79cb42d13d 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -22,12 +22,9 @@ import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import static com.google.common.collect.Iterables.all; @@ -81,7 +78,7 @@ protected EndpointsForRange snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(range); ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) - byEndpoint = this.byEndpoint.subList(newList); + byEndpoint = this.byEndpoint.isSubList(newList); return new EndpointsForRange(range, newList, true, byEndpoint); } diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index 549e52ca280e..73454a20fb92 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -19,15 +19,11 @@ package org.apache.cassandra.locator; import com.google.common.base.Preconditions; -import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; /** @@ -73,7 +69,7 @@ protected EndpointsForToken snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(token); ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) - byEndpoint = this.byEndpoint.subList(newList); + byEndpoint = this.byEndpoint.isSubList(newList); return new EndpointsForToken(token, newList, true, byEndpoint); } diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index f531a18ac8b0..7917ad126f3c 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -99,7 +99,7 @@ protected RangesAtEndpoint snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(endpoint); ReplicaMap> byRange = null; if (this.byRange != null && list.isSubList(newList)) - byRange = this.byRange.subList(newList); + byRange = this.byRange.isSubList(newList); return new RangesAtEndpoint(endpoint, newList, true, byRange); } From ce4f0ccb7559a28dc85ae077abcc9d6bc687e0bd Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Tue, 18 Sep 2018 20:01:05 +0100 Subject: [PATCH 5/8] Remove Mutable, asImmutableView/asSnapshot etc.; use only Builder and build() --- .../apache/cassandra/dht/RangeStreamer.java | 10 +-- .../locator/AbstractReplicaCollection.java | 40 +++++------ .../locator/AbstractReplicationStrategy.java | 10 +-- .../apache/cassandra/locator/Endpoints.java | 23 +++--- .../cassandra/locator/EndpointsByRange.java | 17 +++-- .../cassandra/locator/EndpointsByReplica.java | 19 +++-- .../cassandra/locator/EndpointsForRange.java | 67 +++++++----------- .../cassandra/locator/EndpointsForToken.java | 59 +++++++--------- .../locator/NetworkTopologyStrategy.java | 10 +-- .../locator/OldNetworkTopologyStrategy.java | 4 +- .../cassandra/locator/PendingRangeMaps.java | 38 +++++----- .../cassandra/locator/RangesAtEndpoint.java | 70 ++++++++----------- .../cassandra/locator/RangesByEndpoint.java | 15 ++-- .../cassandra/locator/ReplicaCollection.java | 59 +++++----------- .../cassandra/locator/ReplicaMultimap.java | 23 +++--- .../cassandra/locator/ReplicaPlans.java | 6 +- .../cassandra/locator/SimpleStrategy.java | 7 +- .../cassandra/locator/TokenMetadata.java | 8 +-- .../cassandra/service/RangeRelocator.java | 4 +- .../cassandra/service/StorageService.java | 10 +-- .../dht/RangeFetchMapCalculatorTest.java | 55 +++++++-------- .../locator/ReplicaCollectionTest.java | 27 +++---- .../apache/cassandra/service/MoveTest.java | 5 +- .../cassandra/service/MoveTransientTest.java | 32 ++++----- .../cassandra/service/StorageServiceTest.java | 4 +- .../service/reads/DataResolverTest.java | 5 +- 26 files changed, 287 insertions(+), 340 deletions(-) diff --git a/src/java/org/apache/cassandra/dht/RangeStreamer.java b/src/java/org/apache/cassandra/dht/RangeStreamer.java index b50a4e282e43..75e2530348eb 100644 --- a/src/java/org/apache/cassandra/dht/RangeStreamer.java +++ b/src/java/org/apache/cassandra/dht/RangeStreamer.java @@ -39,7 +39,7 @@ import org.apache.cassandra.locator.EndpointsByRange; import org.apache.cassandra.locator.EndpointsForRange; import org.apache.cassandra.locator.RangesAtEndpoint; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -418,7 +418,7 @@ else if (useStrictConsistency) endpoints -> snitchGetSortedListByProximity.apply(localAddress, endpoints); //This list of replicas is just candidates. With strict consistency it's going to be a narrow list. - EndpointsByReplica.Mutable rangesToFetchWithPreferredEndpoints = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder rangesToFetchWithPreferredEndpoints = new EndpointsByReplica.Builder(); for (Replica toFetch : fetchRanges) { //Replica that is sufficient to provide the data we need @@ -539,7 +539,7 @@ else if (useStrictConsistency) } } } - return rangesToFetchWithPreferredEndpoints.asImmutableView(); + return rangesToFetchWithPreferredEndpoints.build(); } /** @@ -573,14 +573,14 @@ private static Multimap getOptimizedWorkMap(En //the surface area to test and introduce bugs. //In the future it's possible we could run it twice once for full ranges with only full replicas //and once with transient ranges and all replicas. Then merge the result. - EndpointsByRange.Mutable unwrapped = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder unwrapped = new EndpointsByRange.Builder(); for (Map.Entry entry : rangesWithSources.flattenEntries()) { Replicas.temporaryAssertFull(entry.getValue()); unwrapped.put(entry.getKey().range(), entry.getValue()); } - EndpointsByRange unwrappedView = unwrapped.asImmutableView(); + EndpointsByRange unwrappedView = unwrapped.build(); RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(unwrappedView, sourceFilters, keyspace); Multimap> rangeFetchMapMap = calculator.getRangeFetchMap(); logger.info("Output from RangeFetchMapCalculator for keyspace {}", keyspace); diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index bc99414cd3f9..a288bea680dc 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -47,12 +47,12 @@ public abstract class AbstractReplicaCollection, B extends Builder> Collector collector(Set characteristics, Supplier supplier) + public static , B extends Builder> Collector collector(Set characteristics, Supplier supplier) { return new Collector() { private final BiConsumer accumulator = Builder::add; - private final BinaryOperator combiner = (a, b) -> { a.addAll(b.mutable); return a; }; + private final BinaryOperator combiner = (a, b) -> { a.addAll(b); return a; }; private final Function finisher = Builder::build; public Supplier supplier() { return supplier; } public BiConsumer accumulator() { return accumulator; } @@ -230,7 +230,7 @@ protected static class ReplicaMap extends AbstractMap { private final Function toKey; private final ReplicaList list; - // we maintain a map of key -> index in our list; this lets us share with subLists (or between Mutable and snapshots) + // we maintain a map of key -> index in our list; this lets us share with subLists (or between Builder and snapshots) // since we only need to corroborate that the list index we find is within the bounds of our list // (if not, it's a shared map, and the key only occurs in one of our ancestors) private final ObjectIntOpenHashMap map; @@ -359,33 +359,27 @@ ReplicaMap isSubList(ReplicaList subList) } protected final ReplicaList list; - final boolean isSnapshot; - AbstractReplicaCollection(ReplicaList list, boolean isSnapshot) + AbstractReplicaCollection(ReplicaList list) { this.list = list; - this.isSnapshot = isSnapshot; } - // if subList == null, should return self (or a clone thereof) - protected abstract C snapshot(ReplicaList newList); - protected abstract C self(); /** - * construct a new Mutable of our own type, so that we can concatenate + * construct a new Builder of our own type, so that we can concatenate * TODO: this isn't terribly pretty, but we need sometimes to select / merge two Endpoints of unknown type; */ - public abstract Mutable newMutable(int initialCapacity); + public abstract Builder newBuilder(int initialCapacity); - public C snapshot() - { - return isSnapshot ? self() - : snapshot(list.isEmpty() ? EMPTY_LIST - : list); - } + // if subList == null, should return self (or a clone thereof) + abstract C snapshot(ReplicaList newList); + // return this object, if it is an immutable snapshot, otherwise returns a copy with these properties + public abstract C snapshot(); /** see {@link ReplicaCollection#subList(int, int)}*/ public final C subList(int start, int end) { - if (isSnapshot && start == 0 && end == size()) return self(); + if (start == 0 && end == size()) + return snapshot(); ReplicaList subList; if (start == end) subList = EMPTY_LIST; @@ -531,16 +525,16 @@ public final String toString() return Iterables.toString(list); } - static > C concat(C replicas, C extraReplicas, Mutable.Conflict ignoreConflicts) + static > C concat(C replicas, C extraReplicas, Builder.Conflict ignoreConflicts) { if (extraReplicas.isEmpty()) return replicas; if (replicas.isEmpty()) return extraReplicas; - Mutable mutable = replicas.newMutable(replicas.size() + extraReplicas.size()); - mutable.addAll(replicas, Mutable.Conflict.NONE); - mutable.addAll(extraReplicas, ignoreConflicts); - return mutable.asSnapshot(); + Builder builder = replicas.newBuilder(replicas.size() + extraReplicas.size()); + builder.addAll(replicas, Builder.Conflict.NONE); + builder.addAll(extraReplicas, ignoreConflicts); + return builder.build(); } } diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java index bad736fc0581..818e20efe936 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java @@ -23,7 +23,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -223,7 +223,7 @@ public boolean hasTransientReplicas() */ public RangesByEndpoint getAddressReplicas(TokenMetadata metadata) { - RangesByEndpoint.Mutable map = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder map = new RangesByEndpoint.Builder(); for (Token token : metadata.sortedTokens()) { @@ -236,7 +236,7 @@ public RangesByEndpoint getAddressReplicas(TokenMetadata metadata) } } - return map.asImmutableView(); + return map.build(); } public RangesAtEndpoint getAddressReplicas(TokenMetadata metadata, InetAddressAndPort endpoint) @@ -260,7 +260,7 @@ public RangesAtEndpoint getAddressReplicas(TokenMetadata metadata, InetAddressAn public EndpointsByRange getRangeAddresses(TokenMetadata metadata) { - EndpointsByRange.Mutable map = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder map = new EndpointsByRange.Builder(); for (Token token : metadata.sortedTokens()) { @@ -273,7 +273,7 @@ public EndpointsByRange getRangeAddresses(TokenMetadata metadata) } } - return map.asImmutableView(); + return map.build(); } public RangesByEndpoint getAddressReplicas() diff --git a/src/java/org/apache/cassandra/locator/Endpoints.java b/src/java/org/apache/cassandra/locator/Endpoints.java index b27ef1d7eba3..a6694b966114 100644 --- a/src/java/org/apache/cassandra/locator/Endpoints.java +++ b/src/java/org/apache/cassandra/locator/Endpoints.java @@ -18,13 +18,10 @@ package org.apache.cassandra.locator; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.apache.cassandra.utils.FBUtilities; import java.util.Collection; -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -43,9 +40,9 @@ public abstract class Endpoints> extends AbstractReplicaC // besides (transitively) those that cache objects that themselves have only final members ReplicaMap byEndpoint; - Endpoints(ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) + Endpoints(ReplicaList list, ReplicaMap byEndpoint) { - super(list, isSnapshot); + super(list); this.byEndpoint = byEndpoint; } @@ -111,7 +108,7 @@ public E keep(Set keep) */ public E select(Iterable endpoints, boolean ignoreMissing) { - ReplicaCollection.Mutable copy = newMutable( + Builder copy = newBuilder( endpoints instanceof Collection ? ((Collection) endpoints).size() : size() @@ -126,9 +123,9 @@ public E select(Iterable endpoints, boolean ignoreMissing) throw new IllegalArgumentException(endpoint + " is not present in " + this); continue; } - copy.add(select, ReplicaCollection.Mutable.Conflict.DUPLICATE); + copy.add(select, Builder.Conflict.DUPLICATE); } - return copy.asSnapshot(); + return copy.build(); } /** @@ -147,10 +144,10 @@ public static > E concat(E natural, E pending) public static > E append(E replicas, Replica extraReplica) { - Mutable mutable = replicas.newMutable(replicas.size() + 1); - mutable.addAll(replicas); - mutable.add(extraReplica, Conflict.NONE); - return mutable.asSnapshot(); + Builder builder = replicas.newBuilder(replicas.size() + 1); + builder.addAll(replicas); + builder.add(extraReplica, Conflict.NONE); + return builder.build(); } } diff --git a/src/java/org/apache/cassandra/locator/EndpointsByRange.java b/src/java/org/apache/cassandra/locator/EndpointsByRange.java index cdc8a68e370a..fdeb4473c508 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsByRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsByRange.java @@ -22,9 +22,10 @@ import com.google.common.collect.Maps; import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import java.util.Collections; +import java.util.HashMap; import java.util.Map; public class EndpointsByRange extends ReplicaMultimap, EndpointsForRange> @@ -40,12 +41,12 @@ public EndpointsForRange get(Range range) return map.getOrDefault(range, EndpointsForRange.empty(range)); } - public static class Mutable extends ReplicaMultimap.Mutable, EndpointsForRange.Mutable> + public static class Builder extends ReplicaMultimap.Builder, EndpointsForRange.Builder> { @Override - protected EndpointsForRange.Mutable newMutable(Range range) + protected EndpointsForRange.Builder newBuilder(Range range) { - return new EndpointsForRange.Mutable(range); + return new EndpointsForRange.Builder(range); } // TODO: consider all ignoreDuplicates cases @@ -54,9 +55,13 @@ public void putAll(Range range, EndpointsForRange replicas, Conflict igno get(range).addAll(replicas, ignoreConflicts); } - public EndpointsByRange asImmutableView() + public EndpointsByRange build() { - return new EndpointsByRange(Collections.unmodifiableMap(Maps.transformValues(map, EndpointsForRange.Mutable::asImmutableView))); + Map, EndpointsForRange> map = + Collections.unmodifiableMap( + new HashMap<>( + Maps.transformValues(this.map, EndpointsForRange.Builder::build))); + return new EndpointsByRange(map); } } diff --git a/src/java/org/apache/cassandra/locator/EndpointsByReplica.java b/src/java/org/apache/cassandra/locator/EndpointsByReplica.java index ceea2d1183b5..564f80083acc 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsByReplica.java +++ b/src/java/org/apache/cassandra/locator/EndpointsByReplica.java @@ -20,9 +20,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Maps; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import java.util.Collections; +import java.util.HashMap; import java.util.Map; public class EndpointsByReplica extends ReplicaMultimap @@ -38,23 +39,27 @@ public EndpointsForRange get(Replica range) return map.getOrDefault(range, EndpointsForRange.empty(range.range())); } - public static class Mutable extends ReplicaMultimap.Mutable + public static class Builder extends ReplicaMultimap.Builder { @Override - protected EndpointsForRange.Mutable newMutable(Replica replica) + protected EndpointsForRange.Builder newBuilder(Replica replica) { - return new EndpointsForRange.Mutable(replica.range()); + return new EndpointsForRange.Builder(replica.range()); } // TODO: consider all ignoreDuplicates cases public void putAll(Replica range, EndpointsForRange replicas, Conflict ignoreConflicts) { - map.computeIfAbsent(range, r -> newMutable(r)).addAll(replicas, ignoreConflicts); + map.computeIfAbsent(range, r -> newBuilder(r)).addAll(replicas, ignoreConflicts); } - public EndpointsByReplica asImmutableView() + public EndpointsByReplica build() { - return new EndpointsByReplica(Collections.unmodifiableMap(Maps.transformValues(map, EndpointsForRange.Mutable::asImmutableView))); + Map map = + Collections.unmodifiableMap( + new HashMap<>( + Maps.transformValues(this.map, EndpointsForRange.Builder::build))); + return new EndpointsByReplica(map); } } diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index 2a79cb42d13d..ba365635b113 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -37,13 +37,13 @@ public class EndpointsForRange extends Endpoints { private final Range range; - private EndpointsForRange(Range range, ReplicaList list, boolean isSnapshot) + private EndpointsForRange(Range range, ReplicaList list) { - this(range, list, isSnapshot, null); + this(range, list, null); } - private EndpointsForRange(Range range, ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) + private EndpointsForRange(Range range, ReplicaList list, ReplicaMap byEndpoint) { - super(list, isSnapshot, byEndpoint); + super(list, byEndpoint); this.range = range; assert range != null; } @@ -54,44 +54,44 @@ public Range range() } @Override - public Mutable newMutable(int initialCapacity) + public Builder newBuilder(int initialCapacity) { - return new Mutable(range, initialCapacity); + return new Builder(range, initialCapacity); } public EndpointsForToken forToken(Token token) { if (!range.contains(token)) throw new IllegalArgumentException(token + " is not contained within " + range); - return new EndpointsForToken(token, list, isSnapshot, byEndpoint); + return new EndpointsForToken(token, list, byEndpoint); } @Override - public EndpointsForRange self() + public EndpointsForRange snapshot() { return this; } @Override - protected EndpointsForRange snapshot(ReplicaList newList) + EndpointsForRange snapshot(ReplicaList newList) { if (newList.isEmpty()) return empty(range); ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) byEndpoint = this.byEndpoint.isSubList(newList); - return new EndpointsForRange(range, newList, true, byEndpoint); + return new EndpointsForRange(range, newList, byEndpoint); } - public static class Mutable extends EndpointsForRange implements ReplicaCollection.Mutable + public static class Builder extends EndpointsForRange implements ReplicaCollection.Builder { - boolean hasSnapshot; - public Mutable(Range range) { this(range, 0); } - public Mutable(Range range, int capacity) { this(range, new ReplicaList(capacity)); } - private Mutable(Range range, ReplicaList list) { super(range, list, false, endpointMap(list)); } + boolean built; + public Builder(Range range) { this(range, 0); } + public Builder(Range range, int capacity) { this(range, new ReplicaList(capacity)); } + private Builder(Range range, ReplicaList list) { super(range, list, endpointMap(list)); } - public void add(Replica replica, Conflict ignoreConflict) + public EndpointsForRange.Builder add(Replica replica, Conflict ignoreConflict) { - if (hasSnapshot) throw new IllegalStateException(); + if (built) throw new IllegalStateException(); Preconditions.checkNotNull(replica); if (!replica.range().contains(super.range)) throw new IllegalArgumentException("Replica " + replica + " does not contain " + super.range); @@ -108,10 +108,11 @@ public void add(Replica replica, Conflict ignoreConflict) + replica + "; existing: " + byEndpoint().get(replica.endpoint())); case ALL: } - return; + return this; } list.add(replica); + return this; } @Override @@ -122,30 +123,16 @@ public Map byEndpoint() return Collections.unmodifiableMap(super.byEndpoint()); } - private EndpointsForRange get(boolean isSnapshot) - { - return new EndpointsForRange(super.range, super.list, isSnapshot, super.byEndpoint); - } - - public EndpointsForRange asImmutableView() - { - return get(false); - } - - public EndpointsForRange asSnapshot() + @Override + public EndpointsForRange snapshot() { - hasSnapshot = true; - return get(true); + return snapshot(list.subList(0, list.size())); } - } - public static class Builder extends ReplicaCollection.Builder - { - public Builder(Range range) { this(range, 0); } - public Builder(Range range, int capacity) { super (new Mutable(range, capacity)); } - public boolean containsEndpoint(InetAddressAndPort endpoint) + public EndpointsForRange build() { - return mutable.asImmutableView().byEndpoint.containsKey(endpoint); + built = true; + return new EndpointsForRange(super.range, super.list, super.byEndpoint); } } @@ -160,7 +147,7 @@ public static Builder builder(Range range, int capacity) public static EndpointsForRange empty(Range range) { - return new EndpointsForRange(range, EMPTY_LIST, true, EMPTY_MAP); + return new EndpointsForRange(range, EMPTY_LIST, EMPTY_MAP); } public static EndpointsForRange of(Replica replica) @@ -169,7 +156,7 @@ public static EndpointsForRange of(Replica replica) ReplicaList one = new ReplicaList(1); one.add(replica); // we can safely use singletonMap, as we only otherwise use LinkedHashMap - return new EndpointsForRange(replica.range(), one, true, endpointMap(one)); + return new EndpointsForRange(replica.range(), one, endpointMap(one)); } public static EndpointsForRange of(Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index 73454a20fb92..5830c1689b9f 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -34,14 +34,14 @@ public class EndpointsForToken extends Endpoints { private final Token token; - private EndpointsForToken(Token token, ReplicaList list, boolean isSnapshot) + private EndpointsForToken(Token token, ReplicaList list) { - this(token, list, isSnapshot, null); + this(token, list, null); } - EndpointsForToken(Token token, ReplicaList list, boolean isSnapshot, ReplicaMap byEndpoint) + EndpointsForToken(Token token, ReplicaList list, ReplicaMap byEndpoint) { - super(list, isSnapshot, byEndpoint); + super(list, byEndpoint); this.token = token; assert token != null; } @@ -52,13 +52,13 @@ public Token token() } @Override - public Mutable newMutable(int initialCapacity) + public Builder newBuilder(int initialCapacity) { - return new Mutable(token, initialCapacity); + return new Builder(token, initialCapacity); } @Override - public EndpointsForToken self() + public EndpointsForToken snapshot() { return this; } @@ -70,19 +70,19 @@ protected EndpointsForToken snapshot(ReplicaList newList) ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) byEndpoint = this.byEndpoint.isSubList(newList); - return new EndpointsForToken(token, newList, true, byEndpoint); + return new EndpointsForToken(token, newList, byEndpoint); } - public static class Mutable extends EndpointsForToken implements ReplicaCollection.Mutable + public static class Builder extends EndpointsForToken implements ReplicaCollection.Builder { - boolean hasSnapshot; - public Mutable(Token token) { this(token, 0); } - public Mutable(Token token, int capacity) { this(token, new ReplicaList(capacity)); } - private Mutable(Token token, ReplicaList list) { super(token, list, false, endpointMap(list)); } + boolean built; + public Builder(Token token) { this(token, 0); } + public Builder(Token token, int capacity) { this(token, new ReplicaList(capacity)); } + private Builder(Token token, ReplicaList list) { super(token, list, endpointMap(list)); } - public void add(Replica replica, Conflict ignoreConflict) + public EndpointsForToken.Builder add(Replica replica, Conflict ignoreConflict) { - if (hasSnapshot) throw new IllegalStateException(); + if (built) throw new IllegalStateException(); Preconditions.checkNotNull(replica); if (!replica.range().contains(super.token)) throw new IllegalArgumentException("Replica " + replica + " does not contain " + super.token); @@ -99,10 +99,11 @@ public void add(Replica replica, Conflict ignoreConflict) + replica + "; existing: " + byEndpoint().get(replica.endpoint())); case ALL: } - return; + return this; } list.add(replica); + return this; } @Override @@ -113,29 +114,19 @@ public Map byEndpoint() return Collections.unmodifiableMap(super.byEndpoint()); } - private EndpointsForToken get(boolean isSnapshot) - { - return new EndpointsForToken(super.token, super.list, isSnapshot, super.byEndpoint); - } - - public EndpointsForToken asImmutableView() + @Override + public EndpointsForToken snapshot() { - return get(false); + return snapshot(list.subList(0, list.size())); } - public EndpointsForToken asSnapshot() + public EndpointsForToken build() { - hasSnapshot = true; - return get(true); + built = true; + return new EndpointsForToken(super.token, super.list, super.byEndpoint); } } - public static class Builder extends ReplicaCollection.Builder - { - public Builder(Token token) { this(token, 0); } - public Builder(Token token, int capacity) { super (new Mutable(token, capacity)); } - } - public static Builder builder(Token token) { return new Builder(token); @@ -147,7 +138,7 @@ public static Builder builder(Token token, int capacity) public static EndpointsForToken empty(Token token) { - return new EndpointsForToken(token, EMPTY_LIST, true, EMPTY_MAP); + return new EndpointsForToken(token, EMPTY_LIST, EMPTY_MAP); } public static EndpointsForToken of(Token token, Replica replica) @@ -156,7 +147,7 @@ public static EndpointsForToken of(Token token, Replica replica) ReplicaList one = new ReplicaList(1); one.add(replica); // we can safely use singletonMap, as we only otherwise use LinkedHashMap - return new EndpointsForToken(token, one, true, endpointMap(one)); + return new EndpointsForToken(token, one, endpointMap(one)); } public static EndpointsForToken of(Token token, Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java index c63f4f3af83d..713a75eb37cc 100644 --- a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java +++ b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java @@ -20,7 +20,7 @@ import java.util.*; import java.util.Map.Entry; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -87,7 +87,7 @@ public NetworkTopologyStrategy(String keyspaceName, TokenMetadata tokenMetadata, private static final class DatacenterEndpoints { /** List accepted endpoints get pushed into. */ - EndpointsForRange.Mutable replicas; + EndpointsForRange.Builder replicas; /** * Racks encountered so far. Replicas are put into separate racks while possible. @@ -101,7 +101,7 @@ private static final class DatacenterEndpoints int acceptableRackRepeats; int transients; - DatacenterEndpoints(ReplicationFactor rf, int rackCount, int nodeCount, EndpointsForRange.Mutable replicas, Set> racks) + DatacenterEndpoints(ReplicationFactor rf, int rackCount, int nodeCount, EndpointsForRange.Builder replicas, Set> racks) { this.replicas = replicas; this.racks = racks; @@ -168,7 +168,7 @@ public EndpointsForRange calculateNaturalReplicas(Token searchToken, TokenMetada Token replicaStart = tokenMetadata.getPredecessor(replicaEnd); Range replicatedRange = new Range<>(replicaStart, replicaEnd); - EndpointsForRange.Mutable builder = new EndpointsForRange.Mutable(replicatedRange); + EndpointsForRange.Builder builder = new EndpointsForRange.Builder(replicatedRange); Set> seenRacks = new HashSet<>(); Topology topology = tokenMetadata.getTopology(); @@ -206,7 +206,7 @@ public EndpointsForRange calculateNaturalReplicas(Token searchToken, TokenMetada if (dcEndpoints != null && dcEndpoints.addEndpointAndCheckIfDone(ep, location, replicatedRange)) --dcsToFill; } - return builder.asImmutableView(); + return builder.build(); } private int sizeOrZero(Multimap collection) diff --git a/src/java/org/apache/cassandra/locator/OldNetworkTopologyStrategy.java b/src/java/org/apache/cassandra/locator/OldNetworkTopologyStrategy.java index 449c51e28e9a..37152b4e8b63 100644 --- a/src/java/org/apache/cassandra/locator/OldNetworkTopologyStrategy.java +++ b/src/java/org/apache/cassandra/locator/OldNetworkTopologyStrategy.java @@ -54,7 +54,7 @@ public EndpointsForRange calculateNaturalReplicas(Token token, TokenMetadata met Token previousToken = metadata.getPredecessor(primaryToken); Range tokenRange = new Range<>(previousToken, primaryToken); - EndpointsForRange.Builder replicas = EndpointsForRange.builder(tokenRange, rf.allReplicas); + EndpointsForRange.Builder replicas = new EndpointsForRange.Builder(tokenRange, rf.allReplicas); assert !rf.hasTransientReplicas() : "support transient replicas"; replicas.add(new Replica(metadata.getEndpoint(primaryToken), previousToken, primaryToken, true)); @@ -98,7 +98,7 @@ public EndpointsForRange calculateNaturalReplicas(Token token, TokenMetadata met { Token t = iter.next(); Replica replica = new Replica(metadata.getEndpoint(t), previousToken, primaryToken, true); - if (!replicas.containsEndpoint(replica.endpoint())) + if (!replicas.endpoints().contains(replica.endpoint())) replicas.add(replica); } } diff --git a/src/java/org/apache/cassandra/locator/PendingRangeMaps.java b/src/java/org/apache/cassandra/locator/PendingRangeMaps.java index b8b7bc60a095..f9e3f6658c2d 100644 --- a/src/java/org/apache/cassandra/locator/PendingRangeMaps.java +++ b/src/java/org/apache/cassandra/locator/PendingRangeMaps.java @@ -23,11 +23,11 @@ import com.google.common.collect.Iterators; import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import java.util.*; -public class PendingRangeMaps implements Iterable, EndpointsForRange.Mutable>> +public class PendingRangeMaps implements Iterable, EndpointsForRange.Builder>> { /** * We have for NavigableMap to be able to search for ranges containing a token efficiently. @@ -35,7 +35,7 @@ public class PendingRangeMaps implements Iterable, Endpoi * First two are for non-wrap-around ranges, and the last two are for wrap-around ranges. */ // ascendingMap will sort the ranges by the ascending order of right token - private final NavigableMap, EndpointsForRange.Mutable> ascendingMap; + private final NavigableMap, EndpointsForRange.Builder> ascendingMap; /** * sorting end ascending, if ends are same, sorting begin descending, so that token (end, end) will @@ -50,7 +50,7 @@ public class PendingRangeMaps implements Iterable, Endpoi }; // ascendingMap will sort the ranges by the descending order of left token - private final NavigableMap, EndpointsForRange.Mutable> descendingMap; + private final NavigableMap, EndpointsForRange.Builder> descendingMap; /** * sorting begin descending, if begins are same, sorting end descending, so that token (begin, begin) will @@ -66,7 +66,7 @@ public class PendingRangeMaps implements Iterable, Endpoi }; // these two maps are for warp around ranges. - private final NavigableMap, EndpointsForRange.Mutable> ascendingMapForWrapAround; + private final NavigableMap, EndpointsForRange.Builder> ascendingMapForWrapAround; /** * for wrap around range (begin, end], which begin > end. @@ -82,7 +82,7 @@ public class PendingRangeMaps implements Iterable, Endpoi return o1.left.compareTo(o2.left); }; - private final NavigableMap, EndpointsForRange.Mutable> descendingMapForWrapAround; + private final NavigableMap, EndpointsForRange.Builder> descendingMapForWrapAround; /** * for wrap around ranges, which begin > end. @@ -106,13 +106,13 @@ public PendingRangeMaps() static final void addToMap(Range range, Replica replica, - NavigableMap, EndpointsForRange.Mutable> ascendingMap, - NavigableMap, EndpointsForRange.Mutable> descendingMap) + NavigableMap, EndpointsForRange.Builder> ascendingMap, + NavigableMap, EndpointsForRange.Builder> descendingMap) { - EndpointsForRange.Mutable replicas = ascendingMap.get(range); + EndpointsForRange.Builder replicas = ascendingMap.get(range); if (replicas == null) { - replicas = new EndpointsForRange.Mutable(range,1); + replicas = new EndpointsForRange.Builder(range,1); ascendingMap.put(range, replicas); descendingMap.put(range, replicas); } @@ -132,13 +132,13 @@ public void addPendingRange(Range range, Replica replica) } static final void addIntersections(EndpointsForToken.Builder replicasToAdd, - NavigableMap, EndpointsForRange.Mutable> smallerMap, - NavigableMap, EndpointsForRange.Mutable> biggerMap) + NavigableMap, EndpointsForRange.Builder> smallerMap, + NavigableMap, EndpointsForRange.Builder> biggerMap) { // find the intersection of two sets for (Range range : smallerMap.keySet()) { - EndpointsForRange.Mutable replicas = biggerMap.get(range); + EndpointsForRange.Builder replicas = biggerMap.get(range); if (replicas != null) { replicasToAdd.addAll(replicas); @@ -153,8 +153,8 @@ public EndpointsForToken pendingEndpointsFor(Token token) Range searchRange = new Range<>(token, token); // search for non-wrap-around maps - NavigableMap, EndpointsForRange.Mutable> ascendingTailMap = ascendingMap.tailMap(searchRange, true); - NavigableMap, EndpointsForRange.Mutable> descendingTailMap = descendingMap.tailMap(searchRange, false); + NavigableMap, EndpointsForRange.Builder> ascendingTailMap = ascendingMap.tailMap(searchRange, true); + NavigableMap, EndpointsForRange.Builder> descendingTailMap = descendingMap.tailMap(searchRange, false); // add intersections of two maps if (ascendingTailMap.size() < descendingTailMap.size()) @@ -171,11 +171,11 @@ public EndpointsForToken pendingEndpointsFor(Token token) descendingTailMap = descendingMapForWrapAround.tailMap(searchRange, false); // add them since they are all necessary. - for (Map.Entry, EndpointsForRange.Mutable> entry : ascendingTailMap.entrySet()) + for (Map.Entry, EndpointsForRange.Builder> entry : ascendingTailMap.entrySet()) { replicas.addAll(entry.getValue()); } - for (Map.Entry, EndpointsForRange.Mutable> entry : descendingTailMap.entrySet()) + for (Map.Entry, EndpointsForRange.Builder> entry : descendingTailMap.entrySet()) { replicas.addAll(entry.getValue()); } @@ -187,7 +187,7 @@ public String printPendingRanges() { StringBuilder sb = new StringBuilder(); - for (Map.Entry, EndpointsForRange.Mutable> entry : this) + for (Map.Entry, EndpointsForRange.Builder> entry : this) { Range range = entry.getKey(); @@ -202,7 +202,7 @@ public String printPendingRanges() } @Override - public Iterator, EndpointsForRange.Mutable>> iterator() + public Iterator, EndpointsForRange.Builder>> iterator() { return Iterators.concat(ascendingMap.entrySet().iterator(), ascendingMapForWrapAround.entrySet().iterator()); } diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index 7917ad126f3c..d671be03e30d 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -35,7 +35,7 @@ import java.util.stream.Collector; import static com.google.common.collect.Iterables.all; -import static org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict.*; +import static org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict.*; /** * A ReplicaCollection for Ranges occurring at an endpoint. All Replica will be for the same endpoint, @@ -54,13 +54,13 @@ public class RangesAtEndpoint extends AbstractReplicaCollection> byRange) + private RangesAtEndpoint(InetAddressAndPort endpoint, ReplicaList list, ReplicaMap> byRange) { - super(list, isSnapshot); + super(list); this.endpoint = endpoint; this.byRange = byRange; assert endpoint != null; @@ -100,19 +100,19 @@ protected RangesAtEndpoint snapshot(ReplicaList newList) ReplicaMap> byRange = null; if (this.byRange != null && list.isSubList(newList)) byRange = this.byRange.isSubList(newList); - return new RangesAtEndpoint(endpoint, newList, true, byRange); + return new RangesAtEndpoint(endpoint, newList, byRange); } @Override - public RangesAtEndpoint self() + public RangesAtEndpoint snapshot() { return this; } @Override - public ReplicaCollection.Mutable newMutable(int initialCapacity) + public ReplicaCollection.Builder newBuilder(int initialCapacity) { - return new Mutable(endpoint, initialCapacity); + return new Builder(endpoint, initialCapacity); } @Override @@ -182,16 +182,16 @@ public static Collector collector(InetAddres return collector(ImmutableSet.of(), () -> new Builder(endpoint)); } - public static class Mutable extends RangesAtEndpoint implements ReplicaCollection.Mutable + public static class Builder extends RangesAtEndpoint implements ReplicaCollection.Builder { - boolean hasSnapshot; - public Mutable(InetAddressAndPort endpoint) { this(endpoint, 0); } - public Mutable(InetAddressAndPort endpoint, int capacity) { this(endpoint, new ReplicaList(capacity)); } - private Mutable(InetAddressAndPort endpoint, ReplicaList list) { super(endpoint, list, false, rangeMap(list)); } + boolean built; + public Builder(InetAddressAndPort endpoint) { this(endpoint, 0); } + public Builder(InetAddressAndPort endpoint, int capacity) { this(endpoint, new ReplicaList(capacity)); } + private Builder(InetAddressAndPort endpoint, ReplicaList list) { super(endpoint, list, rangeMap(list)); } - public void add(Replica replica, Conflict ignoreConflict) + public RangesAtEndpoint.Builder add(Replica replica, Conflict ignoreConflict) { - if (hasSnapshot) throw new IllegalStateException(); + if (built) throw new IllegalStateException(); Preconditions.checkNotNull(replica); if (!Objects.equals(super.endpoint, replica.endpoint())) throw new IllegalArgumentException("Replica " + replica + " has incorrect endpoint (expected " + super.endpoint + ")"); @@ -208,10 +208,11 @@ public void add(Replica replica, Conflict ignoreConflict) + replica + "; existing: " + byRange().get(replica.range())); case ALL: } - return; + return this; } list.add(replica); + return this; } @Override @@ -222,49 +223,38 @@ public Map, Replica> byRange() return Collections.unmodifiableMap(super.byRange()); } - public RangesAtEndpoint get(boolean isSnapshot) - { - return new RangesAtEndpoint(super.endpoint, super.list, isSnapshot, super.byRange); - } - - public RangesAtEndpoint asImmutableView() + @Override + public RangesAtEndpoint snapshot() { - return get(false); + return snapshot(list.subList(0, list.size())); } - public RangesAtEndpoint asSnapshot() + public RangesAtEndpoint build() { - hasSnapshot = true; - return get(true); + built = true; + return new RangesAtEndpoint(super.endpoint, super.list, super.byRange); } } - public static class Builder extends ReplicaCollection.Builder + public static Builder builder(InetAddressAndPort endpoint) { - public Builder(InetAddressAndPort endpoint) { this(endpoint, 0); } - public Builder(InetAddressAndPort endpoint, int capacity) { super(new Mutable(endpoint, capacity)); } + return new Builder(endpoint); } - - public static RangesAtEndpoint.Builder builder(InetAddressAndPort endpoint) - { - return new RangesAtEndpoint.Builder(endpoint); - } - - public static RangesAtEndpoint.Builder builder(InetAddressAndPort endpoint, int capacity) + public static Builder builder(InetAddressAndPort endpoint, int capacity) { - return new RangesAtEndpoint.Builder(endpoint, capacity); + return new Builder(endpoint, capacity); } public static RangesAtEndpoint empty(InetAddressAndPort endpoint) { - return new RangesAtEndpoint(endpoint, EMPTY_LIST, true, EMPTY_MAP); + return new RangesAtEndpoint(endpoint, EMPTY_LIST, EMPTY_MAP); } public static RangesAtEndpoint of(Replica replica) { ReplicaList one = new ReplicaList(1); one.add(replica); - return new RangesAtEndpoint(replica.endpoint(), one, true, rangeMap(one)); + return new RangesAtEndpoint(replica.endpoint(), one, rangeMap(one)); } public static RangesAtEndpoint of(Replica ... replicas) diff --git a/src/java/org/apache/cassandra/locator/RangesByEndpoint.java b/src/java/org/apache/cassandra/locator/RangesByEndpoint.java index 698b1334d328..9638d49803e9 100644 --- a/src/java/org/apache/cassandra/locator/RangesByEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesByEndpoint.java @@ -22,6 +22,7 @@ import com.google.common.collect.Maps; import java.util.Collections; +import java.util.HashMap; import java.util.Map; public class RangesByEndpoint extends ReplicaMultimap @@ -37,17 +38,21 @@ public RangesAtEndpoint get(InetAddressAndPort endpoint) return map.getOrDefault(endpoint, RangesAtEndpoint.empty(endpoint)); } - public static class Mutable extends ReplicaMultimap.Mutable + public static class Builder extends ReplicaMultimap.Builder { @Override - protected RangesAtEndpoint.Mutable newMutable(InetAddressAndPort endpoint) + protected RangesAtEndpoint.Builder newBuilder(InetAddressAndPort endpoint) { - return new RangesAtEndpoint.Mutable(endpoint); + return new RangesAtEndpoint.Builder(endpoint); } - public RangesByEndpoint asImmutableView() + public RangesByEndpoint build() { - return new RangesByEndpoint(Collections.unmodifiableMap(Maps.transformValues(map, RangesAtEndpoint.Mutable::asImmutableView))); + Map map = + Collections.unmodifiableMap( + new HashMap<>( + Maps.transformValues(this.map, RangesAtEndpoint.Builder::build))); + return new RangesByEndpoint(map); } } diff --git a/src/java/org/apache/cassandra/locator/ReplicaCollection.java b/src/java/org/apache/cassandra/locator/ReplicaCollection.java index b658ba4ce746..d870316b4cae 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/ReplicaCollection.java @@ -18,8 +18,6 @@ package org.apache.cassandra.locator; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; - import java.util.Comparator; import java.util.Iterator; import java.util.Set; @@ -69,14 +67,14 @@ public interface ReplicaCollection> extends Itera /** * @return a *eagerly constructed* copy of this collection containing the Replica that match the provided predicate. * An effort will be made to either return ourself, or a subList, where possible. - * It is guaranteed that no changes to any upstream Mutable will affect the state of the result. + * It is guaranteed that no changes to any upstream Builder will affect the state of the result. */ public abstract C filter(Predicate predicate); /** * @return a *eagerly constructed* copy of this collection containing the Replica that match the provided predicate. * An effort will be made to either return ourself, or a subList, where possible. - * It is guaranteed that no changes to any upstream Mutable will affect the state of the result. + * It is guaranteed that no changes to any upstream Builder will affect the state of the result. * Only the first maxSize items will be returned. */ public abstract C filter(Predicate predicate, int maxSize); @@ -95,13 +93,13 @@ public interface ReplicaCollection> extends Itera /** * @return an *eagerly constructed* copy of this collection containing the Replica at positions [start..end); * An effort will be made to either return ourself, or a subList, where possible. - * It is guaranteed that no changes to any upstream Mutable will affect the state of the result. + * It is guaranteed that no changes to any upstream Builder will affect the state of the result. */ public abstract C subList(int start, int end); /** * @return an *eagerly constructed* copy of this collection containing the Replica re-ordered according to this comparator - * It is guaranteed that no changes to any upstream Mutable will affect the state of the result. + * It is guaranteed that no changes to any upstream Builder will affect the state of the result. */ public abstract C sorted(Comparator comparator); @@ -113,23 +111,18 @@ public interface ReplicaCollection> extends Itera public abstract String toString(); /** - * A mutable extension of a ReplicaCollection. This is append-only, so it is safe to select a subList, - * or at any time take an asImmutableView() snapshot. + * A mutable (append-only) extension of a ReplicaCollection. + * All methods besides add() will return an immutable snapshot of the collection, or the matching items. */ - public interface Mutable> extends ReplicaCollection + public interface Builder> extends ReplicaCollection { /** - * @return an Immutable clone that mirrors any modifications to this Mutable instance. - */ - C asImmutableView(); - - /** - * @return an Immutable clone that assumes this Mutable will never be modified again, + * @return an Immutable clone that assumes this Builder will never be modified again, * so its contents can be reused. * - * This Mutable should enforce that it is no longer modified. + * This Builder should enforce that it is no longer modified. */ - C asSnapshot(); + public C build(); /** * Passed to add() and addAll() as ignoreConflicts parameter. The meaning of conflict varies by collection type @@ -149,41 +142,23 @@ enum Conflict * @param replica add this replica to the end of the collection * @param ignoreConflict conflicts to ignore, see {@link Conflict} */ - void add(Replica replica, Conflict ignoreConflict); + Builder add(Replica replica, Conflict ignoreConflict); - default public void add(Replica replica) + default public Builder add(Replica replica) { - add(replica, Conflict.NONE); + return add(replica, Conflict.NONE); } - default public void addAll(Iterable replicas, Conflict ignoreConflicts) + default public Builder addAll(Iterable replicas, Conflict ignoreConflicts) { for (Replica replica : replicas) add(replica, ignoreConflicts); + return this; } - default public void addAll(Iterable replicas) - { - addAll(replicas, Conflict.NONE); - } - } - - public static class Builder, M extends Mutable, B extends Builder> - { - Mutable mutable; - public Builder(Mutable mutable) { this.mutable = mutable; } - - public int size() { return mutable.size(); } - public B add(Replica replica) { mutable.add(replica); return (B) this; } - public B add(Replica replica, Conflict ignoreConflict) { mutable.add(replica, ignoreConflict); return (B) this; } - public B addAll(Iterable replica) { mutable.addAll(replica); return (B) this; } - public B addAll(Iterable replica, Conflict ignoreConflict) { mutable.addAll(replica, ignoreConflict); return (B) this; } - - public C build() + default public Builder addAll(Iterable replicas) { - C result = mutable.asSnapshot(); - mutable = null; - return result; + return addAll(replicas, Conflict.NONE); } } diff --git a/src/java/org/apache/cassandra/locator/ReplicaMultimap.java b/src/java/org/apache/cassandra/locator/ReplicaMultimap.java index 3e3fcb47353a..fc26bf94cbc2 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaMultimap.java +++ b/src/java/org/apache/cassandra/locator/ReplicaMultimap.java @@ -39,21 +39,28 @@ public abstract class ReplicaMultimap> public abstract C get(K key); public C getIfPresent(K key) { return map.get(key); } - public static abstract class Mutable - > - extends ReplicaMultimap + public static abstract class Builder + > + { - protected abstract MutableCollection newMutable(K key); + protected abstract B newBuilder(K key); - Mutable() + final Map map; + Builder() { - super(new HashMap<>()); + this.map = new HashMap<>(); + } + + public B get(K key) + { + Preconditions.checkNotNull(key); + return map.computeIfAbsent(key, k -> newBuilder(key)); } - public MutableCollection get(K key) + public B getIfPresent(K key) { Preconditions.checkNotNull(key); - return map.computeIfAbsent(key, k -> newMutable(key)); + return map.get(key); } public void put(K key, Replica replica) diff --git a/src/java/org/apache/cassandra/locator/ReplicaPlans.java b/src/java/org/apache/cassandra/locator/ReplicaPlans.java index ed4c5413411e..5551e5fb7355 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaPlans.java +++ b/src/java/org/apache/cassandra/locator/ReplicaPlans.java @@ -41,8 +41,8 @@ import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.reads.AlwaysSpeculativeRetryPolicy; import org.apache.cassandra.service.reads.SpeculativeRetryPolicy; -import org.apache.cassandra.utils.FBUtilities; +import org.apache.cassandra.utils.FBUtilities; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -387,7 +387,7 @@ E select(Keyspace keyspace, ConsistencyLevel consistencyLevel, L liveAndDown, L assert consistencyLevel != EACH_QUORUM; - ReplicaCollection.Mutable contacts = liveAndDown.all().newMutable(liveAndDown.all().size()); + ReplicaCollection.Builder contacts = liveAndDown.all().newBuilder(liveAndDown.all().size()); contacts.addAll(liveAndDown.natural().filterLazily(Replica::isFull)); contacts.addAll(liveAndDown.pending()); @@ -396,7 +396,7 @@ E select(Keyspace keyspace, ConsistencyLevel consistencyLevel, L liveAndDown, L int requiredTransientCount = consistencyLevel.blockForWrite(keyspace, liveAndDown.pending()) - liveCount; if (requiredTransientCount > 0) contacts.addAll(live.natural().filterLazily(Replica::isTransient, requiredTransientCount)); - return contacts.asSnapshot(); + return contacts.build(); } }; diff --git a/src/java/org/apache/cassandra/locator/SimpleStrategy.java b/src/java/org/apache/cassandra/locator/SimpleStrategy.java index d924ac5682fd..2dd835c26120 100644 --- a/src/java/org/apache/cassandra/locator/SimpleStrategy.java +++ b/src/java/org/apache/cassandra/locator/SimpleStrategy.java @@ -57,20 +57,21 @@ public EndpointsForRange calculateNaturalReplicas(Token token, TokenMetadata met Range replicaRange = new Range<>(replicaStart, replicaEnd); Iterator iter = TokenMetadata.ringIterator(ring, token, false); - EndpointsForRange.Builder replicas = EndpointsForRange.builder(replicaRange, rf.allReplicas); + EndpointsForRange.Builder replicas = new EndpointsForRange.Builder(replicaRange, rf.allReplicas); // Add the token at the index by default while (replicas.size() < rf.allReplicas && iter.hasNext()) { Token tk = iter.next(); InetAddressAndPort ep = metadata.getEndpoint(tk); - if (!replicas.containsEndpoint(ep)) + if (!replicas.endpoints().contains(ep)) replicas.add(new Replica(ep, replicaRange, replicas.size() < rf.fullReplicas)); } // group endpoints by DC, so that we can cheaply filter them to a given DC IEndpointSnitch snitch = DatabaseDescriptor.getEndpointSnitch(); - return replicas.build().sorted(Comparator.comparing(r -> snitch.getDatacenter(r.endpoint()))); + return replicas.build() + .sorted(Comparator.comparing(r -> snitch.getDatacenter(r.endpoint()))); } public ReplicationFactor getReplicationFactor() diff --git a/src/java/org/apache/cassandra/locator/TokenMetadata.java b/src/java/org/apache/cassandra/locator/TokenMetadata.java index ad40d7bb3bb1..f95ee0caa3cb 100644 --- a/src/java/org/apache/cassandra/locator/TokenMetadata.java +++ b/src/java/org/apache/cassandra/locator/TokenMetadata.java @@ -27,7 +27,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.*; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -737,18 +737,18 @@ public ArrayList sortedTokens() public EndpointsByRange getPendingRangesMM(String keyspaceName) { - EndpointsByRange.Mutable byRange = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder byRange = new EndpointsByRange.Builder(); PendingRangeMaps pendingRangeMaps = this.pendingRanges.get(keyspaceName); if (pendingRangeMaps != null) { - for (Map.Entry, EndpointsForRange.Mutable> entry : pendingRangeMaps) + for (Map.Entry, EndpointsForRange.Builder> entry : pendingRangeMaps) { byRange.putAll(entry.getKey(), entry.getValue(), Conflict.ALL); } } - return byRange.asImmutableView(); + return byRange.build(); } /** a mutable map may be returned but caller should not modify it */ diff --git a/src/java/org/apache/cassandra/service/RangeRelocator.java b/src/java/org/apache/cassandra/service/RangeRelocator.java index 839a34c11de8..b63c105bd2f5 100644 --- a/src/java/org/apache/cassandra/service/RangeRelocator.java +++ b/src/java/org/apache/cassandra/service/RangeRelocator.java @@ -114,7 +114,7 @@ public static RangesByEndpoint calculateRangesToStreamWithEndpoints(RangesAtEndp TokenMetadata tmdBefore, TokenMetadata tmdAfter) { - RangesByEndpoint.Mutable endpointRanges = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder endpointRanges = new RangesByEndpoint.Builder(); for (Replica toStream : streamRanges) { //If the range we are sending is full only send it to the new full replica @@ -158,7 +158,7 @@ public static RangesByEndpoint calculateRangesToStreamWithEndpoints(RangesAtEndp } } } - return endpointRanges.asImmutableView(); + return endpointRanges.build(); } public void calculateToFromStreams() diff --git a/src/java/org/apache/cassandra/service/StorageService.java b/src/java/org/apache/cassandra/service/StorageService.java index 5536f173e201..abc189e2644a 100644 --- a/src/java/org/apache/cassandra/service/StorageService.java +++ b/src/java/org/apache/cassandra/service/StorageService.java @@ -47,7 +47,7 @@ import com.google.common.util.concurrent.*; import org.apache.cassandra.dht.RangeStreamer.FetchReplica; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; @@ -2975,7 +2975,7 @@ static EndpointsByReplica getChangedReplicasForLeaving(String keyspaceName, Inet if (temp.isMember(endpoint)) temp.removeEndpoint(endpoint); - EndpointsByReplica.Mutable changedRanges = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder changedRanges = new EndpointsByReplica.Builder(); // Go through the ranges and for each range check who will be // storing replicas for these ranges when the leaving endpoint @@ -3011,7 +3011,7 @@ static EndpointsByReplica getChangedReplicasForLeaving(String keyspaceName, Inet changedRanges.putAll(replica, newReplicaEndpoints, Conflict.NONE); } - return changedRanges.asImmutableView(); + return changedRanges.build(); } public void onJoin(InetAddressAndPort endpoint, EndpointState epState) @@ -5028,7 +5028,7 @@ private Future streamRanges(Map rangesT Map>> transferredRangePerKeyspace = SystemKeyspace.getTransferredRanges("Unbootstrap", keyspace, StorageService.instance.getTokenMetadata().partitioner); - RangesByEndpoint.Mutable replicasPerEndpoint = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder replicasPerEndpoint = new RangesByEndpoint.Builder(); for (Map.Entry endPointEntry : rangesWithEndpoints.flattenEntries()) { Replica local = endPointEntry.getKey(); @@ -5043,7 +5043,7 @@ private Future streamRanges(Map rangesT replicasPerEndpoint.put(remote.endpoint(), remote.decorateSubrange(local.range())); } - sessionsToStreamByKeyspace.put(keyspace, replicasPerEndpoint.asImmutableView()); + sessionsToStreamByKeyspace.put(keyspace, replicasPerEndpoint.build()); } StreamPlan streamPlan = new StreamPlan(StreamOperation.DECOMMISSION); diff --git a/test/unit/org/apache/cassandra/dht/RangeFetchMapCalculatorTest.java b/test/unit/org/apache/cassandra/dht/RangeFetchMapCalculatorTest.java index cee4bb99740a..e2b09bc9ee71 100644 --- a/test/unit/org/apache/cassandra/dht/RangeFetchMapCalculatorTest.java +++ b/test/unit/org/apache/cassandra/dht/RangeFetchMapCalculatorTest.java @@ -36,7 +36,6 @@ import org.apache.cassandra.locator.AbstractNetworkTopologySnitch; import org.apache.cassandra.locator.InetAddressAndPort; import org.apache.cassandra.locator.Replica; -import org.apache.cassandra.locator.ReplicaUtils; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -81,14 +80,14 @@ private int getIPLastPart(InetAddressAndPort endpoint) @Test public void testWithSingleSource() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.2"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 31, 40, "127.0.0.4"); addNonTrivialRangeAndSources(rangesWithSources, 41, 50, "127.0.0.5"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -98,14 +97,14 @@ public void testWithSingleSource() throws Exception @Test public void testWithNonOverlappingSource() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1", "127.0.0.2"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.3", "127.0.0.4"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.5", "127.0.0.6"); addNonTrivialRangeAndSources(rangesWithSources, 31, 40, "127.0.0.7", "127.0.0.8"); addNonTrivialRangeAndSources(rangesWithSources, 41, 50, "127.0.0.9", "127.0.0.10"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -115,12 +114,12 @@ public void testWithNonOverlappingSource() throws Exception @Test public void testWithRFThreeReplacement() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1", "127.0.0.2"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.2", "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.3", "127.0.0.4"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -131,14 +130,14 @@ public void testWithRFThreeReplacement() throws Exception @Test public void testForMultipleRoundsComputation() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 31, 40, "127.0.0.3"); addNonTrivialRangeAndSources(rangesWithSources, 41, 50, "127.0.0.3", "127.0.0.2"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -153,14 +152,14 @@ public void testForMultipleRoundsComputation() throws Exception @Test public void testForMultipleRoundsComputationWithLocalHost() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 31, 40, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 41, 50, "127.0.0.1", "127.0.0.2"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -173,14 +172,14 @@ public void testForMultipleRoundsComputationWithLocalHost() throws Exception @Test public void testForEmptyGraph() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 31, 40, "127.0.0.1"); addNonTrivialRangeAndSources(rangesWithSources, 41, 50, "127.0.0.1"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> map = calculator.getRangeFetchMap(); //All ranges map to local host so we will not stream anything. assertTrue(map.isEmpty()); @@ -189,7 +188,7 @@ public void testForEmptyGraph() throws Exception @Test public void testWithNoSourceWithLocal() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1", "127.0.0.5"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.2"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.3"); @@ -218,7 +217,7 @@ public String message(Replica replica) } }; - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Arrays.asList(filter), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Arrays.asList(filter), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); @@ -233,7 +232,7 @@ public String message(Replica replica) @Test (expected = IllegalStateException.class) public void testWithNoLiveSource() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.5"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.2"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.3"); @@ -251,19 +250,19 @@ public String message(Replica replica) } }; - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Arrays.asList(allDeadFilter), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Arrays.asList(allDeadFilter), "Test"); calculator.getRangeFetchMap(); } @Test public void testForLocalDC() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.1", "127.0.0.3", "127.0.0.53"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.1", "127.0.0.3", "127.0.0.57"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.2", "127.0.0.59", "127.0.0.61"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), new ArrayList<>(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), new ArrayList<>(), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); Assert.assertEquals(2, map.asMap().size()); @@ -276,7 +275,7 @@ public void testForLocalDC() throws Exception @Test public void testForRemoteDC() throws Exception { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.3", "127.0.0.51"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.3", "127.0.0.55"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.2", "127.0.0.59"); @@ -305,7 +304,7 @@ public String message(Replica replica) } }; - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Arrays.asList(localHostFilter), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Arrays.asList(localHostFilter), "Test"); Multimap> map = calculator.getRangeFetchMap(); validateRange(rangesWithSources, map); Assert.assertEquals(3, map.asMap().size()); @@ -319,14 +318,14 @@ public String message(Replica replica) @Test public void testTrivialRanges() throws UnknownHostException { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); // add non-trivial ranges addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.3", "127.0.0.51"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.3", "127.0.0.55"); addNonTrivialRangeAndSources(rangesWithSources, 21, 30, "127.0.0.2", "127.0.0.59"); // and a trivial one: addTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.3", "127.0.0.51"); - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.emptyList(), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.emptyList(), "Test"); Multimap> optMap = calculator.getRangeFetchMapForNonTrivialRanges(); Multimap> trivialMap = calculator.getRangeFetchMapForTrivialRanges(optMap); assertTrue(trivialMap.get(InetAddressAndPort.getByName("127.0.0.3")).contains(generateTrivialRange(1,10)) ^ @@ -337,7 +336,7 @@ public void testTrivialRanges() throws UnknownHostException @Test(expected = IllegalStateException.class) public void testNotEnoughEndpointsForTrivialRange() throws UnknownHostException { - EndpointsByRange.Mutable rangesWithSources = new EndpointsByRange.Mutable(); + EndpointsByRange.Builder rangesWithSources = new EndpointsByRange.Builder(); // add non-trivial ranges addNonTrivialRangeAndSources(rangesWithSources, 1, 10, "127.0.0.3", "127.0.0.51"); addNonTrivialRangeAndSources(rangesWithSources, 11, 20, "127.0.0.3", "127.0.0.55"); @@ -366,7 +365,7 @@ public String message(Replica replica) return "Not 127.0.0.3"; } }; - RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.asImmutableView(), Collections.singleton(filter), "Test"); + RangeFetchMapCalculator calculator = new RangeFetchMapCalculator(rangesWithSources.build(), Collections.singleton(filter), "Test"); Multimap> optMap = calculator.getRangeFetchMapForNonTrivialRanges(); Multimap> trivialMap = calculator.getRangeFetchMapForTrivialRanges(optMap); @@ -378,7 +377,7 @@ private void assertArrays(Collection> expected, Collection> result) + private void validateRange(EndpointsByRange.Builder rangesWithSources, Multimap> result) { for (Map.Entry> entry : result.entries()) { @@ -386,7 +385,7 @@ private void validateRange(EndpointsByRange.Mutable rangesWithSources, Multimap< } } - private void addNonTrivialRangeAndSources(EndpointsByRange.Mutable rangesWithSources, int left, int right, String... hosts) throws UnknownHostException + private void addNonTrivialRangeAndSources(EndpointsByRange.Builder rangesWithSources, int left, int right, String... hosts) throws UnknownHostException { for (InetAddressAndPort endpoint : makeAddrs(hosts)) { @@ -395,7 +394,7 @@ private void addNonTrivialRangeAndSources(EndpointsByRange.Mutable rangesWithSou } } - private void addTrivialRangeAndSources(EndpointsByRange.Mutable rangesWithSources, int left, int right, String... hosts) throws UnknownHostException + private void addTrivialRangeAndSources(EndpointsByRange.Builder rangesWithSources, int left, int right, String... hosts) throws UnknownHostException { for (InetAddressAndPort endpoint : makeAddrs(hosts)) { diff --git a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java index 3dce1413fc9a..8b1f826b9ee9 100644 --- a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java +++ b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java @@ -27,7 +27,7 @@ import org.apache.cassandra.dht.Murmur3Partitioner; import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; -import org.apache.cassandra.locator.ReplicaCollection.Mutable.Conflict; +import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; import org.apache.cassandra.utils.FBUtilities; import org.junit.Assert; import org.junit.Test; @@ -147,7 +147,6 @@ public void testOrderOfIteration() private void assertSubList(C subCollection, int from, int to) { - Assert.assertTrue(subCollection.isSnapshot); if (from == to) { Assert.assertTrue(subCollection.isEmpty()); @@ -155,7 +154,7 @@ private void assertSubList(C subCollection, int from, int to) else { AbstractReplicaCollection.ReplicaList subList = this.test.list.subList(from, to); - if (test.isSnapshot) + if (!(test instanceof ReplicaCollection.Builder)) Assert.assertSame(subList.contents, subCollection.list.contents); Assert.assertEquals(subList, subCollection.list); } @@ -173,7 +172,7 @@ private void assertSubSequence(Iterable subSequence, int from, int to) void testSubList(int subListDepth, int filterDepth, int sortDepth) { - if (test.isSnapshot) + if (!(test instanceof ReplicaCollection.Builder)) Assert.assertSame(test, test.subList(0, test.size())); if (test.isEmpty()) @@ -189,7 +188,7 @@ void testSubList(int subListDepth, int filterDepth, int sortDepth) void testFilter(int subListDepth, int filterDepth, int sortDepth) { - if (test.isSnapshot) + if (!(test instanceof ReplicaCollection.Builder)) Assert.assertSame(test, test.filter(Predicates.alwaysTrue())); if (test.isEmpty()) @@ -380,7 +379,7 @@ public void testRangesAtEndpoint() public void testMutableRangesAtEndpoint() { ImmutableList canonical1 = RANGES_AT_ENDPOINT.subList(0, RANGES_AT_ENDPOINT.size()); - RangesAtEndpoint.Mutable test = new RangesAtEndpoint.Mutable(RANGES_AT_ENDPOINT.get(0).endpoint(), canonical1.size()); + RangesAtEndpoint.Builder test = new RangesAtEndpoint.Builder(RANGES_AT_ENDPOINT.get(0).endpoint(), canonical1.size()); test.addAll(canonical1, Conflict.NONE); try { // incorrect range @@ -402,13 +401,11 @@ public void testMutableRangesAtEndpoint() new RangesAtEndpointTestCase(test, canonical1).testAll(); - RangesAtEndpoint view = test.asImmutableView(); - RangesAtEndpoint snapshot = view.subList(0, view.size()); + RangesAtEndpoint snapshot = test.subList(0, test.size()); ImmutableList canonical2 = RANGES_AT_ENDPOINT; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(view, canonical2).testAll(); new TestCase<>(test, canonical2).testAll(); } @@ -433,7 +430,7 @@ public void testEndpointsForRange() public void testMutableEndpointsForRange() { ImmutableList canonical1 = ENDPOINTS_FOR_X.subList(0, ENDPOINTS_FOR_X.size() - 1); - EndpointsForRange.Mutable test = new EndpointsForRange.Mutable(R1, canonical1.size()); + EndpointsForRange.Builder test = new EndpointsForRange.Builder(R1, canonical1.size()); test.addAll(canonical1, Conflict.NONE); try { // incorrect range @@ -455,13 +452,11 @@ public void testMutableEndpointsForRange() new TestCase<>(test, canonical1).testAll(); - EndpointsForRange view = test.asImmutableView(); - EndpointsForRange snapshot = view.subList(0, view.size()); + EndpointsForRange snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(view, canonical2).testAll(); new TestCase<>(test, canonical2).testAll(); } @@ -478,7 +473,7 @@ public void testEndpointsForToken() public void testMutableEndpointsForToken() { ImmutableList canonical1 = ENDPOINTS_FOR_X.subList(0, ENDPOINTS_FOR_X.size() - 1); - EndpointsForToken.Mutable test = new EndpointsForToken.Mutable(tk(1), canonical1.size()); + EndpointsForToken.Builder test = new EndpointsForToken.Builder(tk(1), canonical1.size()); test.addAll(canonical1, Conflict.NONE); try { // incorrect range @@ -500,13 +495,11 @@ public void testMutableEndpointsForToken() new TestCase<>(test, canonical1).testAll(); - EndpointsForToken view = test.asImmutableView(); - EndpointsForToken snapshot = view.subList(0, view.size()); + EndpointsForToken snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(view, canonical2).testAll(); new TestCase<>(test, canonical2).testAll(); } } diff --git a/test/unit/org/apache/cassandra/service/MoveTest.java b/test/unit/org/apache/cassandra/service/MoveTest.java index 731a25d40788..a7cfc1bddf32 100644 --- a/test/unit/org/apache/cassandra/service/MoveTest.java +++ b/test/unit/org/apache/cassandra/service/MoveTest.java @@ -37,7 +37,6 @@ import org.apache.cassandra.locator.InetAddressAndPort; import org.apache.cassandra.locator.Replica; import org.apache.cassandra.locator.ReplicaCollection; -import org.apache.cassandra.locator.ReplicaUtils; import org.apache.cassandra.schema.MigrationManager; import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.config.DatabaseDescriptor; @@ -535,10 +534,10 @@ private void assertPendingRanges(TokenMetadata tmd, Map, EndpointsF private void assertMaps(Map, EndpointsForRange> expected, PendingRangeMaps actual) { int sizeOfActual = 0; - Iterator, EndpointsForRange.Mutable>> iterator = actual.iterator(); + Iterator, EndpointsForRange.Builder>> iterator = actual.iterator(); while(iterator.hasNext()) { - Map.Entry, EndpointsForRange.Mutable> actualEntry = iterator.next(); + Map.Entry, EndpointsForRange.Builder> actualEntry = iterator.next(); assertNotNull(expected.get(actualEntry.getKey())); assertEquals(ImmutableSet.copyOf(expected.get(actualEntry.getKey())), ImmutableSet.copyOf(actualEntry.getValue())); sizeOfActual++; diff --git a/test/unit/org/apache/cassandra/service/MoveTransientTest.java b/test/unit/org/apache/cassandra/service/MoveTransientTest.java index e5a63c7d20cf..a3868a234267 100644 --- a/test/unit/org/apache/cassandra/service/MoveTransientTest.java +++ b/test/unit/org/apache/cassandra/service/MoveTransientTest.java @@ -392,7 +392,7 @@ private Pair constructTMDsMoveForward() @Test public void testMoveForwardBetweenCalculateRangesToFetchWithPreferredEndpoints() throws Exception { - EndpointsByReplica.Mutable expectedResult = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder expectedResult = new EndpointsByReplica.Builder(); InetAddressAndPort cOrB = (downNodes.contains(address03) || sourceFilterDownNodes.contains(address03)) ? address02 : address03; @@ -406,7 +406,7 @@ public void testMoveForwardBetweenCalculateRangesToFetchWithPreferredEndpoints() invokeCalculateRangesToFetchWithPreferredEndpoints(calculateStreamAndFetchRangesMoveForwardBetween().right, constructTMDsMoveForwardBetween(), - expectedResult.asImmutableView()); + expectedResult.build()); } @Test @@ -469,7 +469,7 @@ public void testMoveForwardBetweenCalculateRangesToFetchWithPreferredEndpointsDo @Test public void testMoveBackwardBetweenCalculateRangesToFetchWithPreferredEndpoints() throws Exception { - EndpointsByReplica.Mutable expectedResult = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder expectedResult = new EndpointsByReplica.Builder(); //Need to pull the full replica and the transient replica that is losing the range expectedResult.put(fullReplica(address01, nineToken, elevenToken), fullReplica(address05, nineToken, elevenToken)); @@ -477,7 +477,7 @@ public void testMoveBackwardBetweenCalculateRangesToFetchWithPreferredEndpoints( invokeCalculateRangesToFetchWithPreferredEndpoints(calculateStreamAndFetchRangesMoveBackwardBetween().right, constructTMDsMoveBackwardBetween(), - expectedResult.asImmutableView()); + expectedResult.build()); } @@ -503,18 +503,18 @@ public void testMoveBackwardBetweenCalculateRangesToFetchWithPreferredEndpointsD public void testMoveBackwardCalculateRangesToFetchWithPreferredEndpoints() throws Exception { //Moving backwards should fetch nothing and fetch ranges is emptys so this doesn't test a ton - EndpointsByReplica.Mutable expectedResult = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder expectedResult = new EndpointsByReplica.Builder(); invokeCalculateRangesToFetchWithPreferredEndpoints(calculateStreamAndFetchRangesMoveBackward().right, constructTMDsMoveBackward(), - expectedResult.asImmutableView()); + expectedResult.build()); } @Test public void testMoveForwardCalculateRangesToFetchWithPreferredEndpoints() throws Exception { - EndpointsByReplica.Mutable expectedResult = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder expectedResult = new EndpointsByReplica.Builder(); InetAddressAndPort cOrBAddress = (downNodes.contains(address03) || sourceFilterDownNodes.contains(address03)) ? address02 : address03; @@ -524,7 +524,7 @@ public void testMoveForwardCalculateRangesToFetchWithPreferredEndpoints() throws invokeCalculateRangesToFetchWithPreferredEndpoints(calculateStreamAndFetchRangesMoveForward().right, constructTMDsMoveForward(), - expectedResult.asImmutableView()); + expectedResult.build()); } @@ -626,7 +626,7 @@ public String getDatacenter(InetAddressAndPort endpoint) public void testMoveForwardBetweenCalculateRangesToStreamWithPreferredEndpoints() throws Exception { DatabaseDescriptor.setTransientReplicationEnabledUnsafe(true); - RangesByEndpoint.Mutable expectedResult = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder expectedResult = new RangesByEndpoint.Builder(); //Need to pull the full replica and the transient replica that is losing the range expectedResult.put(address02, transientReplica(address02, nineToken, elevenToken)); @@ -634,13 +634,13 @@ public void testMoveForwardBetweenCalculateRangesToStreamWithPreferredEndpoints( invokeCalculateRangesToStreamWithPreferredEndpoints(calculateStreamAndFetchRangesMoveForwardBetween().left, constructTMDsMoveForwardBetween(), - expectedResult.asImmutableView()); + expectedResult.build()); } @Test public void testMoveBackwardBetweenCalculateRangesToStreamWithPreferredEndpoints() throws Exception { - RangesByEndpoint.Mutable expectedResult = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder expectedResult = new RangesByEndpoint.Builder(); expectedResult.put(address02, fullReplica(address02, fourteenToken, oneToken)); @@ -651,30 +651,30 @@ public void testMoveBackwardBetweenCalculateRangesToStreamWithPreferredEndpoints invokeCalculateRangesToStreamWithPreferredEndpoints(calculateStreamAndFetchRangesMoveBackwardBetween().left, constructTMDsMoveBackwardBetween(), - expectedResult.asImmutableView()); + expectedResult.build()); } @Test public void testMoveBackwardCalculateRangesToStreamWithPreferredEndpoints() throws Exception { - RangesByEndpoint.Mutable expectedResult = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder expectedResult = new RangesByEndpoint.Builder(); expectedResult.put(address03, fullReplica(address03, twoToken, threeToken)); expectedResult.put(address04, transientReplica(address04, twoToken, threeToken)); invokeCalculateRangesToStreamWithPreferredEndpoints(calculateStreamAndFetchRangesMoveBackward().left, constructTMDsMoveBackward(), - expectedResult.asImmutableView()); + expectedResult.build()); } @Test public void testMoveForwardCalculateRangesToStreamWithPreferredEndpoints() throws Exception { //Nothing to stream moving forward because we are acquiring more range not losing range - RangesByEndpoint.Mutable expectedResult = new RangesByEndpoint.Mutable(); + RangesByEndpoint.Builder expectedResult = new RangesByEndpoint.Builder(); invokeCalculateRangesToStreamWithPreferredEndpoints(calculateStreamAndFetchRangesMoveForward().left, constructTMDsMoveForward(), - expectedResult.asImmutableView()); + expectedResult.build()); } private void invokeCalculateRangesToStreamWithPreferredEndpoints(RangesAtEndpoint toStream, diff --git a/test/unit/org/apache/cassandra/service/StorageServiceTest.java b/test/unit/org/apache/cassandra/service/StorageServiceTest.java index cc7fac3d6655..f22f89fc52d8 100644 --- a/test/unit/org/apache/cassandra/service/StorageServiceTest.java +++ b/test/unit/org/apache/cassandra/service/StorageServiceTest.java @@ -149,12 +149,12 @@ public void testGetChangedReplicasForLeaving() throws Exception EndpointsByReplica result = StorageService.getChangedReplicasForLeaving("StorageServiceTest", aAddress, tmd, strat); System.out.println(result); - EndpointsByReplica.Mutable expectedResult = new EndpointsByReplica.Mutable(); + EndpointsByReplica.Builder expectedResult = new EndpointsByReplica.Builder(); expectedResult.put(new Replica(aAddress, aRange, true), new Replica(cAddress, new Range<>(oneToken, sixToken), true)); expectedResult.put(new Replica(aAddress, aRange, true), new Replica(dAddress, new Range<>(oneToken, sixToken), false)); expectedResult.put(new Replica(aAddress, eRange, true), new Replica(bAddress, eRange, true)); expectedResult.put(new Replica(aAddress, eRange, true), new Replica(cAddress, eRange, false)); expectedResult.put(new Replica(aAddress, dRange, false), new Replica(bAddress, dRange, false)); - assertMultimapEqualsIgnoreOrder(result, expectedResult.asImmutableView()); + assertMultimapEqualsIgnoreOrder(result, expectedResult.build()); } } diff --git a/test/unit/org/apache/cassandra/service/reads/DataResolverTest.java b/test/unit/org/apache/cassandra/service/reads/DataResolverTest.java index c49bf3a83bde..17c6e41f4c0f 100644 --- a/test/unit/org/apache/cassandra/service/reads/DataResolverTest.java +++ b/test/unit/org/apache/cassandra/service/reads/DataResolverTest.java @@ -57,7 +57,6 @@ import org.apache.cassandra.locator.EndpointsForRange; import org.apache.cassandra.locator.InetAddressAndPort; import org.apache.cassandra.locator.Replica; -import org.apache.cassandra.locator.ReplicaLayout; import org.apache.cassandra.locator.ReplicaUtils; import org.apache.cassandra.net.*; import org.apache.cassandra.service.reads.repair.ReadRepair; @@ -1186,10 +1185,10 @@ public void responsesFromOlderVersionsAreNotTracked() public void responsesFromTransientReplicasAreNotTracked() { EndpointsForRange replicas = makeReplicas(2); - EndpointsForRange.Mutable mutable = replicas.newMutable(2); + EndpointsForRange.Builder mutable = replicas.newBuilder(2); mutable.add(replicas.get(0)); mutable.add(Replica.transientReplica(replicas.get(1).endpoint(), replicas.range())); - replicas = mutable.asSnapshot(); + replicas = mutable.build(); TestRepairedDataVerifier verifier = new TestRepairedDataVerifier(); ByteBuffer digest1 = ByteBufferUtil.bytes("digest1"); From 106ea015dc30563f54245d5be4948b6f359f8b95 Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Wed, 19 Sep 2018 10:11:14 +0100 Subject: [PATCH 6/8] minor cleanup --- .../apache/cassandra/locator/AbstractReplicaCollection.java | 4 ++-- src/java/org/apache/cassandra/locator/EndpointsForRange.java | 2 +- src/java/org/apache/cassandra/locator/EndpointsForToken.java | 2 +- src/java/org/apache/cassandra/locator/RangesAtEndpoint.java | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index a288bea680dc..16eee68c6f67 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -351,7 +351,7 @@ public int size() return list.size(); } - ReplicaMap isSubList(ReplicaList subList) + ReplicaMap forSubList(ReplicaList subList) { assert subList.contents == list.contents; return new ReplicaMap<>(subList, toKey, map); @@ -370,7 +370,7 @@ ReplicaMap isSubList(ReplicaList subList) */ public abstract Builder newBuilder(int initialCapacity); - // if subList == null, should return self (or a clone thereof) + // return a new "sub-collection" with some sub-selection of the contents of this collection abstract C snapshot(ReplicaList newList); // return this object, if it is an immutable snapshot, otherwise returns a copy with these properties public abstract C snapshot(); diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index ba365635b113..3df0a6bef9e8 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -78,7 +78,7 @@ EndpointsForRange snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(range); ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) - byEndpoint = this.byEndpoint.isSubList(newList); + byEndpoint = this.byEndpoint.forSubList(newList); return new EndpointsForRange(range, newList, byEndpoint); } diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index 5830c1689b9f..b5426c0a2fda 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -69,7 +69,7 @@ protected EndpointsForToken snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(token); ReplicaMap byEndpoint = null; if (this.byEndpoint != null && list.isSubList(newList)) - byEndpoint = this.byEndpoint.isSubList(newList); + byEndpoint = this.byEndpoint.forSubList(newList); return new EndpointsForToken(token, newList, byEndpoint); } diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index d671be03e30d..2ebb620371a3 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -99,7 +99,7 @@ protected RangesAtEndpoint snapshot(ReplicaList newList) if (newList.isEmpty()) return empty(endpoint); ReplicaMap> byRange = null; if (this.byRange != null && list.isSubList(newList)) - byRange = this.byRange.isSubList(newList); + byRange = this.byRange.forSubList(newList); return new RangesAtEndpoint(endpoint, newList, byRange); } From 299df9d9b71bb47927d30bb7a12da2419926532b Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Wed, 19 Sep 2018 11:00:59 +0100 Subject: [PATCH 7/8] improve tests; remove unnecessary Collections.unmodifiableMap calls / method overrides --- .../locator/AbstractReplicaCollection.java | 4 + .../cassandra/locator/EndpointsForRange.java | 8 -- .../cassandra/locator/EndpointsForToken.java | 8 -- .../cassandra/locator/RangesAtEndpoint.java | 8 -- .../locator/ReplicaCollectionTest.java | 128 ++++++++++++++++-- 5 files changed, 119 insertions(+), 37 deletions(-) diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index 16eee68c6f67..7e56eb8c0e4f 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -20,6 +20,7 @@ import com.carrotsearch.hppc.ObjectIntOpenHashMap; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.collect.Iterables; import java.util.AbstractMap; @@ -260,6 +261,7 @@ class EntrySet extends AbstractImmutableSet> @Override public boolean contains(Object o) { + Preconditions.checkNotNull(o); if (!(o instanceof Entry)) return false; return Objects.equals(get(((Entry) o).getKey()), ((Entry) o).getValue()); } @@ -309,11 +311,13 @@ public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHas @Override public boolean containsKey(Object key) { + Preconditions.checkNotNull(key); return get((K)key) != null; } public Replica get(Object key) { + Preconditions.checkNotNull(key); int index = map.get((K)key) - 1; // since this map can be shared between sublists (or snapshots of mutables) // we have to first corroborate that the index we've found is actually within our list's bounds diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index 3df0a6bef9e8..4b2dc02a14fd 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -115,14 +115,6 @@ public EndpointsForRange.Builder add(Replica replica, Conflict ignoreConflict) return this; } - @Override - public Map byEndpoint() - { - // our internal map is modifiable, but it is unsafe to modify the map externally - // it would be possible to implement a safe modifiable map, but it is probably not valuable - return Collections.unmodifiableMap(super.byEndpoint()); - } - @Override public EndpointsForRange snapshot() { diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index b5426c0a2fda..92f91a01c869 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -106,14 +106,6 @@ public EndpointsForToken.Builder add(Replica replica, Conflict ignoreConflict) return this; } - @Override - public Map byEndpoint() - { - // our internal map is modifiable, but it is unsafe to modify the map externally - // it would be possible to implement a safe modifiable map, but it is probably not valuable - return Collections.unmodifiableMap(super.byEndpoint()); - } - @Override public EndpointsForToken snapshot() { diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index 2ebb620371a3..cdd870219e07 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -215,14 +215,6 @@ public RangesAtEndpoint.Builder add(Replica replica, Conflict ignoreConflict) return this; } - @Override - public Map, Replica> byRange() - { - // our internal map is modifiable, but it is unsafe to modify the map externally - // it would be possible to implement a safe modifiable map, but it is probably not valuable - return Collections.unmodifiableMap(super.byRange()); - } - @Override public RangesAtEndpoint snapshot() { diff --git a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java index 8b1f826b9ee9..48b9b3758d12 100644 --- a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java +++ b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java @@ -22,6 +22,7 @@ import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import org.apache.cassandra.dht.Murmur3Partitioner; @@ -34,6 +35,7 @@ import java.net.UnknownHostException; import java.util.ArrayList; +import java.util.AbstractMap; import java.util.Comparator; import java.util.LinkedHashSet; import java.util.List; @@ -50,7 +52,8 @@ public class ReplicaCollectionTest static final InetAddressAndPort EP1, EP2, EP3, EP4, EP5, BROADCAST_EP, NULL_EP; static final Range R1, R2, R3, R4, R5, BROADCAST_RANGE, NULL_RANGE; - + static final List ALL_EP; + static final List> ALL_R; static { try @@ -69,6 +72,8 @@ public class ReplicaCollectionTest R5 = range(4, 0); BROADCAST_RANGE = range(10, 11); NULL_RANGE = range(10000, 10001); + ALL_EP = ImmutableList.of(EP1, EP2, EP3, EP4, EP5, BROADCAST_EP); + ALL_R = ImmutableList.of(R1, R2, R3, R4, R5, BROADCAST_RANGE); } catch (UnknownHostException e) { @@ -133,7 +138,7 @@ void testEndpoints() Assert.assertTrue(test.endpoints().containsAll(canonicalByEndpoint.keySet())); for (InetAddressAndPort ep : canonicalByEndpoint.keySet()) Assert.assertTrue(test.endpoints().contains(ep)); - for (InetAddressAndPort ep : ImmutableList.of(EP1, EP2, EP3, EP4, EP5, BROADCAST_EP)) + for (InetAddressAndPort ep : ALL_EP) if (!canonicalByEndpoint.containsKey(ep)) Assert.assertFalse(test.endpoints().contains(ep)); } @@ -142,7 +147,7 @@ public void testOrderOfIteration() { Assert.assertEquals(canonicalList, ImmutableList.copyOf(test)); Assert.assertEquals(canonicalList, test.stream().collect(Collectors.toList())); - Assert.assertEquals(new LinkedHashSet<>(Lists.transform(canonicalList, Replica::endpoint)), test.endpoints()); + Assert.assertTrue(Iterables.elementsEqual(new LinkedHashSet<>(Lists.transform(canonicalList, Replica::endpoint)), test.endpoints())); } private void assertSubList(C subCollection, int from, int to) @@ -315,16 +320,53 @@ void testRanges() Assert.assertTrue(test.ranges().containsAll(canonicalByRange.keySet())); for (Range range : canonicalByRange.keySet()) Assert.assertTrue(test.ranges().contains(range)); - for (Range range : ImmutableList.of(R1, R2, R3, R4, R5, BROADCAST_RANGE)) + for (Range range : ALL_R) if (!canonicalByRange.containsKey(range)) Assert.assertFalse(test.ranges().contains(range)); } + void testByRange() + { + // check byEndppint() and byRange().entrySet() + Assert.assertFalse(test.byRange().containsKey(EP1)); + Assert.assertFalse(test.byRange().entrySet().contains(EP1)); + try + { + test.byRange().entrySet().contains(null); + Assert.fail(); + } catch (NullPointerException | IllegalArgumentException e) {} + try + { + test.byRange().containsKey(null); + Assert.fail(); + } catch (NullPointerException | IllegalArgumentException e) {} + + for (Range r : ALL_R) + { + if (canonicalByRange.containsKey(r)) + { + Assert.assertTrue(test.byRange().containsKey(r)); + Assert.assertEquals(canonicalByRange.get(r), ImmutableSet.of(test.byRange().get(r))); + for (Replica replica : canonicalByRange.get(r)) + Assert.assertTrue(test.byRange().entrySet().contains(new AbstractMap.SimpleImmutableEntry<>(r, replica))); + } + else + { + Assert.assertFalse(test.byRange().containsKey(r)); + Assert.assertFalse(test.byRange().entrySet().contains(new AbstractMap.SimpleImmutableEntry<>(r, Replica.fullReplica(EP1, r)))); + } + } + } + @Override public void testOrderOfIteration() { super.testOrderOfIteration(); - Assert.assertEquals(new LinkedHashSet<>(Lists.transform(canonicalList, Replica::range)), test.ranges()); + Assert.assertTrue(Iterables.elementsEqual(Lists.transform(canonicalList, Replica::range), test.ranges())); + Assert.assertTrue(Iterables.elementsEqual(canonicalList, test.byRange().values())); + Assert.assertTrue(Iterables.elementsEqual( + Lists.transform(canonicalList, r -> new AbstractMap.SimpleImmutableEntry<>(r.range(), r)), + test.byRange().entrySet())); } public void testUnwrap(int subListDepth, int filterDepth, int sortDepth) @@ -349,8 +391,10 @@ void testAllExceptUnwrap(int subListDepth, int filterDepth, int sortDepth) { super.testAll(subListDepth, filterDepth, sortDepth); testRanges(); + testByRange(); } + @Override void testAll(int subListDepth, int filterDepth, int sortDepth) { testAllExceptUnwrap(subListDepth, filterDepth, sortDepth); @@ -358,6 +402,64 @@ void testAll(int subListDepth, int filterDepth, int sortDepth) } } + static class EndpointsTestCase> extends TestCase + { + EndpointsTestCase(E test, List canonicalList) + { + super(test, canonicalList); + } + + void testByEndpoint() + { + // check byEndppint() and byEndpoint().entrySet() + Assert.assertFalse(test.byEndpoint().containsKey(R1)); + Assert.assertFalse(test.byEndpoint().entrySet().contains(EP1)); + try + { + test.byEndpoint().entrySet().contains(null); + Assert.fail(); + } catch (NullPointerException | IllegalArgumentException e) {} + try + { + test.byEndpoint().containsKey(null); + Assert.fail(); + } catch (NullPointerException | IllegalArgumentException e) {} + + for (InetAddressAndPort ep : ALL_EP) + { + if (canonicalByEndpoint.containsKey(ep)) + { + Assert.assertTrue(test.byEndpoint().containsKey(ep)); + Assert.assertEquals(canonicalByEndpoint.get(ep), ImmutableSet.of(test.byEndpoint().get(ep))); + for (Replica replica : canonicalByEndpoint.get(ep)) + Assert.assertTrue(test.byEndpoint().entrySet().contains(new AbstractMap.SimpleImmutableEntry<>(ep, replica))); + } + else + { + Assert.assertFalse(test.byEndpoint().containsKey(ep)); + Assert.assertFalse(test.byEndpoint().entrySet().contains(new AbstractMap.SimpleImmutableEntry<>(ep, Replica.fullReplica(ep, R1)))); + } + } + } + + @Override + public void testOrderOfIteration() + { + super.testOrderOfIteration(); + Assert.assertTrue(Iterables.elementsEqual(canonicalList, test.byEndpoint().values())); + Assert.assertTrue(Iterables.elementsEqual( + Lists.transform(canonicalList, r -> new AbstractMap.SimpleImmutableEntry<>(r.endpoint(), r)), + test.byEndpoint().entrySet())); + } + + @Override + void testAll(int subListDepth, int filterDepth, int sortDepth) + { + super.testAll(subListDepth, filterDepth, sortDepth); + testByEndpoint(); + } + } + private static final ImmutableList RANGES_AT_ENDPOINT = ImmutableList.of( fullReplica(EP1, R1), fullReplica(EP1, R2), @@ -421,7 +523,7 @@ public void testMutableRangesAtEndpoint() public void testEndpointsForRange() { ImmutableList canonical = ENDPOINTS_FOR_X; - new TestCase<>( + new EndpointsTestCase<>( EndpointsForRange.copyOf(canonical), canonical ).testAll(); } @@ -450,21 +552,21 @@ public void testMutableEndpointsForRange() } catch (IllegalArgumentException e) { } test.add(transientReplica(EP1, R1), Conflict.ALL); - new TestCase<>(test, canonical1).testAll(); + new EndpointsTestCase<>(test, canonical1).testAll(); EndpointsForRange snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); - new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(test, canonical2).testAll(); + new EndpointsTestCase<>(snapshot, canonical1).testAll(); + new EndpointsTestCase<>(test, canonical2).testAll(); } @Test public void testEndpointsForToken() { ImmutableList canonical = ENDPOINTS_FOR_X; - new TestCase<>( + new EndpointsTestCase<>( EndpointsForToken.copyOf(tk(1), canonical), canonical ).testAll(); } @@ -493,13 +595,13 @@ public void testMutableEndpointsForToken() } catch (IllegalArgumentException e) { } test.add(transientReplica(EP1, R1), Conflict.ALL); - new TestCase<>(test, canonical1).testAll(); + new EndpointsTestCase<>(test, canonical1).testAll(); EndpointsForToken snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); - new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(test, canonical2).testAll(); + new EndpointsTestCase<>(snapshot, canonical1).testAll(); + new EndpointsTestCase<>(test, canonical2).testAll(); } } From 5b41c59bda57c7304a04dc33aefe9ef4671832db Mon Sep 17 00:00:00 2001 From: Benedict Elliott Smith Date: Mon, 1 Oct 2018 09:42:49 +0100 Subject: [PATCH 8/8] address review comments --- .../locator/AbstractReplicaCollection.java | 48 ++++++++-------- .../apache/cassandra/locator/Endpoints.java | 2 +- .../cassandra/locator/EndpointsByRange.java | 9 ++- .../cassandra/locator/EndpointsByReplica.java | 9 ++- .../cassandra/locator/EndpointsForRange.java | 6 -- .../cassandra/locator/EndpointsForToken.java | 6 -- .../cassandra/locator/RangesAtEndpoint.java | 12 ++-- .../cassandra/locator/RangesByEndpoint.java | 9 ++- .../cassandra/locator/ReplicaMultimap.java | 3 +- .../locator/ReplicaCollectionTest.java | 55 ++++++++++--------- 10 files changed, 79 insertions(+), 80 deletions(-) diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java index 7e56eb8c0e4f..374afc6f6b8e 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicaCollection.java @@ -93,7 +93,9 @@ public Replica get(int index) public void add(Replica replica) { // can only add to full array - if we have sliced it, we must be a snapshot - assert begin == 0; + if (begin != 0) + throw new IllegalStateException(); + if (size == contents.length) { int newSize; @@ -133,6 +135,8 @@ public Stream stream() return Arrays.stream(contents, begin, begin + size); } + // we implement our own iterator, because it is trivial to do so, and in monomorphic call sites + // will compile down to almost optimal indexed for loop @Override public Iterator iterator() { @@ -149,11 +153,14 @@ public boolean hasNext() @Override public Replica next() { + if (!hasNext()) throw new IllegalStateException(); return contents[i++]; } }; } + // we implement our own iterator, because it is trivial to do so, and in monomorphic call sites + // will compile down to almost optimal indexed for loop public Iterator transformIterator(Function function) { return new Iterator() @@ -174,6 +181,9 @@ public K next() }; } + // we implement our own iterator, because it is trivial to do so, and in monomorphic call sites + // will compile down to almost optimal indexed for loop + // in this case, especially, it is impactful versus Iterables.limit(Iterables.filter()) private Iterator filterIterator(Predicate predicate, int limit) { return new Iterator() @@ -213,10 +223,7 @@ public boolean equals(Object to) return false; ReplicaList that = (ReplicaList) to; if (this.size != that.size) return false; - for (int i = 0 ; i < size ; ++i) - if (!this.get(i).equals(that.get(i))) - return false; - return true; + return Iterables.elementsEqual(this, that); } } @@ -238,7 +245,7 @@ protected static class ReplicaMap extends AbstractMap private Set keySet; private Set> entrySet; - abstract class AbstractImmutableSet extends AbstractSet + abstract class AbstractImmutableSet extends AbstractSet { @Override public boolean removeAll(Collection c) { throw new UnsupportedOperationException(); } @@ -273,29 +280,18 @@ public Iterator> iterator() } } - private static boolean putIfAbsent(ObjectIntOpenHashMap map, Function toKey, Replica replica, int index) - { - K key = toKey.apply(replica); - int otherIndex = map.put(key, index + 1); - if (otherIndex == 0) - return true; - map.put(key, otherIndex); - return false; - } - public ReplicaMap(ReplicaList list, Function toKey) { // 8*0.65 => RF=5; 16*0.65 ==> RF=10 // use list capacity if empty, otherwise use actual list size - ObjectIntOpenHashMap map = new ObjectIntOpenHashMap<>(list.size == 0 ? list.contents.length : list.size, 0.65f); + this.toKey = toKey; + this.map = new ObjectIntOpenHashMap<>(list.size == 0 ? list.contents.length : list.size, 0.65f); + this.list = list; for (int i = list.begin ; i < list.begin + list.size ; ++i) { - boolean inserted = putIfAbsent(map, toKey, list.contents[i], i); + boolean inserted = internalPutIfAbsent(list.contents[i], i); assert inserted; } - this.toKey = toKey; - this.list = list; - this.map = map; } public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHashMap map) @@ -306,7 +302,15 @@ public ReplicaMap(ReplicaList list, Function toKey, ObjectIntOpenHas } // to be used only by subclasses of AbstractReplicaCollection - boolean internalPutIfAbsent(Replica replica, int index) { return putIfAbsent(map, toKey, replica, index); } + boolean internalPutIfAbsent(Replica replica, int index) + { + K key = toKey.apply(replica); + int otherIndex = map.put(key, index + 1); + if (otherIndex == 0) + return true; + map.put(key, otherIndex); + return false; + } @Override public boolean containsKey(Object key) diff --git a/src/java/org/apache/cassandra/locator/Endpoints.java b/src/java/org/apache/cassandra/locator/Endpoints.java index a6694b966114..ee42e3679c48 100644 --- a/src/java/org/apache/cassandra/locator/Endpoints.java +++ b/src/java/org/apache/cassandra/locator/Endpoints.java @@ -33,7 +33,7 @@ */ public abstract class Endpoints> extends AbstractReplicaCollection { - static final ReplicaMap endpointMap(ReplicaList list) { return new ReplicaMap<>(list, Replica::endpoint); } + static ReplicaMap endpointMap(ReplicaList list) { return new ReplicaMap<>(list, Replica::endpoint); } static final ReplicaMap EMPTY_MAP = endpointMap(EMPTY_LIST); // volatile not needed, as has only final members, diff --git a/src/java/org/apache/cassandra/locator/EndpointsByRange.java b/src/java/org/apache/cassandra/locator/EndpointsByRange.java index fdeb4473c508..2d1cde609016 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsByRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsByRange.java @@ -19,6 +19,7 @@ package org.apache.cassandra.locator; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; @@ -57,11 +58,9 @@ public void putAll(Range range, EndpointsForRange replicas, Conflict igno public EndpointsByRange build() { - Map, EndpointsForRange> map = - Collections.unmodifiableMap( - new HashMap<>( - Maps.transformValues(this.map, EndpointsForRange.Builder::build))); - return new EndpointsByRange(map); + return new EndpointsByRange( + ImmutableMap.copyOf( + Maps.transformValues(this.map, EndpointsForRange.Builder::build))); } } diff --git a/src/java/org/apache/cassandra/locator/EndpointsByReplica.java b/src/java/org/apache/cassandra/locator/EndpointsByReplica.java index 564f80083acc..72d8751374e0 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsByReplica.java +++ b/src/java/org/apache/cassandra/locator/EndpointsByReplica.java @@ -19,6 +19,7 @@ package org.apache.cassandra.locator; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import org.apache.cassandra.locator.ReplicaCollection.Builder.Conflict; @@ -55,11 +56,9 @@ public void putAll(Replica range, EndpointsForRange replicas, Conflict ignoreCon public EndpointsByReplica build() { - Map map = - Collections.unmodifiableMap( - new HashMap<>( - Maps.transformValues(this.map, EndpointsForRange.Builder::build))); - return new EndpointsByReplica(map); + return new EndpointsByReplica( + ImmutableMap.copyOf( + Maps.transformValues(this.map, EndpointsForRange.Builder::build))); } } diff --git a/src/java/org/apache/cassandra/locator/EndpointsForRange.java b/src/java/org/apache/cassandra/locator/EndpointsForRange.java index 4b2dc02a14fd..7039df055f5c 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForRange.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForRange.java @@ -24,8 +24,6 @@ import java.util.Arrays; import java.util.Collection; -import java.util.Collections; -import java.util.Map; import static com.google.common.collect.Iterables.all; @@ -37,10 +35,6 @@ public class EndpointsForRange extends Endpoints { private final Range range; - private EndpointsForRange(Range range, ReplicaList list) - { - this(range, list, null); - } private EndpointsForRange(Range range, ReplicaList list, ReplicaMap byEndpoint) { super(list, byEndpoint); diff --git a/src/java/org/apache/cassandra/locator/EndpointsForToken.java b/src/java/org/apache/cassandra/locator/EndpointsForToken.java index 92f91a01c869..c709988762a6 100644 --- a/src/java/org/apache/cassandra/locator/EndpointsForToken.java +++ b/src/java/org/apache/cassandra/locator/EndpointsForToken.java @@ -23,8 +23,6 @@ import java.util.Arrays; import java.util.Collection; -import java.util.Collections; -import java.util.Map; /** * A ReplicaCollection where all Replica are required to cover a range that fully contains the token() defined in the builder(). @@ -34,10 +32,6 @@ public class EndpointsForToken extends Endpoints { private final Token token; - private EndpointsForToken(Token token, ReplicaList list) - { - this(token, list, null); - } EndpointsForToken(Token token, ReplicaList list, ReplicaMap byEndpoint) { diff --git a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java index cdd870219e07..33ddffd818f4 100644 --- a/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesAtEndpoint.java @@ -54,10 +54,6 @@ public class RangesAtEndpoint extends AbstractReplicaCollection> byRange) { super(list); @@ -80,11 +76,19 @@ public Set endpoints() ); } + /** + * @return a set of all unique Ranges + * This method is threadsafe, though it is not synchronised + */ public Set> ranges() { return byRange().keySet(); } + /** + * @return a map of all Ranges, to their owning Replica instance + * This method is threadsafe, though it is not synchronised + */ public Map, Replica> byRange() { ReplicaMap> map = byRange; diff --git a/src/java/org/apache/cassandra/locator/RangesByEndpoint.java b/src/java/org/apache/cassandra/locator/RangesByEndpoint.java index 9638d49803e9..1a711418a7b2 100644 --- a/src/java/org/apache/cassandra/locator/RangesByEndpoint.java +++ b/src/java/org/apache/cassandra/locator/RangesByEndpoint.java @@ -19,6 +19,7 @@ package org.apache.cassandra.locator; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.util.Collections; @@ -48,11 +49,9 @@ protected RangesAtEndpoint.Builder newBuilder(InetAddressAndPort endpoint) public RangesByEndpoint build() { - Map map = - Collections.unmodifiableMap( - new HashMap<>( - Maps.transformValues(this.map, RangesAtEndpoint.Builder::build))); - return new RangesByEndpoint(map); + return new RangesByEndpoint( + ImmutableMap.copyOf( + Maps.transformValues(this.map, RangesAtEndpoint.Builder::build))); } } diff --git a/src/java/org/apache/cassandra/locator/ReplicaMultimap.java b/src/java/org/apache/cassandra/locator/ReplicaMultimap.java index fc26bf94cbc2..5a8551ad6683 100644 --- a/src/java/org/apache/cassandra/locator/ReplicaMultimap.java +++ b/src/java/org/apache/cassandra/locator/ReplicaMultimap.java @@ -25,9 +25,11 @@ import java.util.Set; import java.util.stream.Stream; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Iterables; +@VisibleForTesting public abstract class ReplicaMultimap> { final Map map; @@ -37,7 +39,6 @@ public abstract class ReplicaMultimap> } public abstract C get(K key); - public C getIfPresent(K key) { return map.get(key); } public static abstract class Builder > diff --git a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java index 48b9b3758d12..ced49e2fa4b5 100644 --- a/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java +++ b/test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java @@ -93,13 +93,15 @@ static Range range(long left, long right) static class TestCase> { + final boolean isBuilder; final C test; final List canonicalList; final Multimap canonicalByEndpoint; final Multimap, Replica> canonicalByRange; - TestCase(C test, List canonicalList) + TestCase(boolean isBuilder, C test, List canonicalList) { + this.isBuilder = isBuilder; this.test = test; this.canonicalList = canonicalList; this.canonicalByEndpoint = HashMultimap.create(); @@ -108,6 +110,8 @@ static class TestCase> canonicalByEndpoint.put(replica.endpoint(), replica); for (Replica replica : canonicalList) canonicalByRange.put(replica.range(), replica); + if (isBuilder) + Assert.assertTrue(test instanceof ReplicaCollection.Builder); } void testSize() @@ -159,7 +163,7 @@ private void assertSubList(C subCollection, int from, int to) else { AbstractReplicaCollection.ReplicaList subList = this.test.list.subList(from, to); - if (!(test instanceof ReplicaCollection.Builder)) + if (!isBuilder) Assert.assertSame(subList.contents, subCollection.list.contents); Assert.assertEquals(subList, subCollection.list); } @@ -177,23 +181,24 @@ private void assertSubSequence(Iterable subSequence, int from, int to) void testSubList(int subListDepth, int filterDepth, int sortDepth) { - if (!(test instanceof ReplicaCollection.Builder)) + if (!isBuilder) Assert.assertSame(test, test.subList(0, test.size())); if (test.isEmpty()) return; - TestCase skipFront = new TestCase<>(test.subList(1, test.size()), canonicalList.subList(1, canonicalList.size())); + Assert.assertSame(test.list.contents, test.subList(0, 1).list.contents); + TestCase skipFront = new TestCase<>(false, test.subList(1, test.size()), canonicalList.subList(1, canonicalList.size())); assertSubList(skipFront.test, 1, canonicalList.size()); skipFront.testAll(subListDepth - 1, filterDepth, sortDepth); - TestCase skipBack = new TestCase<>(test.subList(0, test.size() - 1), canonicalList.subList(0, canonicalList.size() - 1)); + TestCase skipBack = new TestCase<>(false, test.subList(0, test.size() - 1), canonicalList.subList(0, canonicalList.size() - 1)); assertSubList(skipBack.test, 0, canonicalList.size() - 1); skipBack.testAll(subListDepth - 1, filterDepth, sortDepth); } void testFilter(int subListDepth, int filterDepth, int sortDepth) { - if (!(test instanceof ReplicaCollection.Builder)) + if (!isBuilder) Assert.assertSame(test, test.filter(Predicates.alwaysTrue())); if (test.isEmpty()) @@ -225,7 +230,7 @@ void testFilter(int subListDepth, int filterDepth, int sortDepth) return; Predicate removeMiddle = r -> !r.equals(canonicalList.get(canonicalList.size() / 2)); - TestCase filtered = new TestCase<>(test.filter(removeMiddle), ImmutableList.copyOf(filter(canonicalList, removeMiddle::test))); + TestCase filtered = new TestCase<>(false, test.filter(removeMiddle), ImmutableList.copyOf(filter(canonicalList, removeMiddle::test))); filtered.testAll(subListDepth, filterDepth - 1, sortDepth); Assert.assertTrue(elementsEqual(filtered.canonicalList, test.filterLazily(removeMiddle, Integer.MAX_VALUE))); Assert.assertTrue(elementsEqual(limit(filter(canonicalList, removeMiddle::test), canonicalList.size() - 2), test.filterLazily(removeMiddle, canonicalList.size() - 2))); @@ -269,7 +274,7 @@ void testSort(int subListDepth, int filterDepth, int sortDepth) boolean f2 = o2.equals(canonicalList.get(0)); return f1 == f2 ? 0 : f1 ? 1 : -1; }; - TestCase sorted = new TestCase<>(test.sorted(comparator), ImmutableList.sortedCopyOf(comparator, canonicalList)); + TestCase sorted = new TestCase<>(false, test.sorted(comparator), ImmutableList.sortedCopyOf(comparator, canonicalList)); sorted.testAll(subListDepth, filterDepth, sortDepth - 1); } @@ -298,9 +303,9 @@ public void testAll() static class RangesAtEndpointTestCase extends TestCase { - RangesAtEndpointTestCase(RangesAtEndpoint test, List canonicalList) + RangesAtEndpointTestCase(boolean isBuilder, RangesAtEndpoint test, List canonicalList) { - super(test, canonicalList); + super(isBuilder, test, canonicalList); } void testRanges() @@ -382,7 +387,7 @@ public void testUnwrap(int subListDepth, int filterDepth, int sortDepth) } else { - new RangesAtEndpointTestCase(testUnwrap, canonUnwrap) + new RangesAtEndpointTestCase(false, testUnwrap, canonUnwrap) .testAllExceptUnwrap(subListDepth, filterDepth, sortDepth); } } @@ -404,9 +409,9 @@ void testAll(int subListDepth, int filterDepth, int sortDepth) static class EndpointsTestCase> extends TestCase { - EndpointsTestCase(E test, List canonicalList) + EndpointsTestCase(boolean isBuilder, E test, List canonicalList) { - super(test, canonicalList); + super(isBuilder, test, canonicalList); } void testByEndpoint() @@ -473,7 +478,7 @@ public void testRangesAtEndpoint() { ImmutableList canonical = RANGES_AT_ENDPOINT; new RangesAtEndpointTestCase( - RangesAtEndpoint.copyOf(canonical), canonical + false, RangesAtEndpoint.copyOf(canonical), canonical ).testAll(); } @@ -501,14 +506,14 @@ public void testMutableRangesAtEndpoint() } catch (IllegalArgumentException e) { } test.add(fullReplica(EP1, R3), Conflict.ALL); - new RangesAtEndpointTestCase(test, canonical1).testAll(); + new RangesAtEndpointTestCase(true, test, canonical1).testAll(); RangesAtEndpoint snapshot = test.subList(0, test.size()); ImmutableList canonical2 = RANGES_AT_ENDPOINT; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); - new TestCase<>(snapshot, canonical1).testAll(); - new TestCase<>(test, canonical2).testAll(); + new TestCase<>(false, snapshot, canonical1).testAll(); + new TestCase<>(true, test, canonical2).testAll(); } private static final ImmutableList ENDPOINTS_FOR_X = ImmutableList.of( @@ -524,7 +529,7 @@ public void testEndpointsForRange() { ImmutableList canonical = ENDPOINTS_FOR_X; new EndpointsTestCase<>( - EndpointsForRange.copyOf(canonical), canonical + false, EndpointsForRange.copyOf(canonical), canonical ).testAll(); } @@ -552,14 +557,14 @@ public void testMutableEndpointsForRange() } catch (IllegalArgumentException e) { } test.add(transientReplica(EP1, R1), Conflict.ALL); - new EndpointsTestCase<>(test, canonical1).testAll(); + new EndpointsTestCase<>(true, test, canonical1).testAll(); EndpointsForRange snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); - new EndpointsTestCase<>(snapshot, canonical1).testAll(); - new EndpointsTestCase<>(test, canonical2).testAll(); + new EndpointsTestCase<>(false, snapshot, canonical1).testAll(); + new EndpointsTestCase<>(true, test, canonical2).testAll(); } @Test @@ -567,7 +572,7 @@ public void testEndpointsForToken() { ImmutableList canonical = ENDPOINTS_FOR_X; new EndpointsTestCase<>( - EndpointsForToken.copyOf(tk(1), canonical), canonical + false, EndpointsForToken.copyOf(tk(1), canonical), canonical ).testAll(); } @@ -595,13 +600,13 @@ public void testMutableEndpointsForToken() } catch (IllegalArgumentException e) { } test.add(transientReplica(EP1, R1), Conflict.ALL); - new EndpointsTestCase<>(test, canonical1).testAll(); + new EndpointsTestCase<>(true, test, canonical1).testAll(); EndpointsForToken snapshot = test.subList(0, test.size()); ImmutableList canonical2 = ENDPOINTS_FOR_X; test.addAll(canonical2.reverse(), Conflict.DUPLICATE); - new EndpointsTestCase<>(snapshot, canonical1).testAll(); - new EndpointsTestCase<>(test, canonical2).testAll(); + new EndpointsTestCase<>(false, snapshot, canonical1).testAll(); + new EndpointsTestCase<>(true, test, canonical2).testAll(); } }