bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1387295720
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends
CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new
TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that
most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is
in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
can you take a look at how we handled this in the prune deps patch? Instead
of adding an actual cache, there's a check to determine if we can expose
objects not specified in the preload context. Then we can have it return
true/false based on some random distribution. Aside from having a lower
footprint, I think it would simulate a wider variety of eviction patterns.
See
https://github.com/bdeggleston/cassandra-accord/blob/C18784-reduce-deps-v2/accord-core/src/main/java/accord/impl/InMemoryCommandStore.java#L759C51-L759C51
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp
executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid
quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they
should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch =
TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()),
txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure)
-> {
Review Comment:
wouldn't we want to exclude the now unreplicated keys from the route to
prevent invalidate from running into mismatch issues itself?
##########
accord-core/src/main/java/accord/coordinate/TopologyMismatch.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package accord.coordinate;
+
+import java.util.EnumSet;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.Routables;
+import accord.primitives.Seekables;
+import accord.primitives.TxnId;
+import accord.topology.Topology;
+
+public class TopologyMismatch extends CoordinationFailed
+{
+ public final Topology topology;
+ public final Seekables<?, ?> keysOrRanges;
+ private final EnumSet<Reason> reasons;
+
+ public enum Reason { HOME_KEY, KEYS_OR_RANGES }
+
+ private TopologyMismatch(EnumSet<Reason> reasons, Topology topology, TxnId
txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ super(txnId, homeKey, buildMessage(reasons, topology, homeKey,
keysOrRanges));
+ this.reasons = reasons;
+ this.topology = topology;
+ this.keysOrRanges = keysOrRanges;
+ }
+
+ private static String buildMessage(EnumSet<Reason> reason, Topology
topology, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ StringBuilder sb = new StringBuilder();
+ if (reason.contains(Reason.KEYS_OR_RANGES))
+ sb.append(String.format("Txn attempted to access keys or ranges %s
that are not longer valid globally (%d -> %s)", keysOrRanges, topology.epoch(),
topology.ranges()));
+ if (reason.contains(Reason.HOME_KEY))
+ {
+ if (sb.length() != 0)
+ sb.append('\n');
+ sb.append(String.format("HomeKey %s exists for a range that is no
longer globally valid (%d -> %s)", homeKey, topology.epoch(),
topology.ranges()));
+ }
+ return sb.toString();
+ }
+
+ @Nullable
+ public static TopologyMismatch checkForMismatch(Topology t, TxnId txnId,
RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ EnumSet<TopologyMismatch.Reason> reasons = null;
+ if (!t.ranges().contains(homeKey))
+ {
+ if (reasons == null)
+ reasons = EnumSet.noneOf(TopologyMismatch.Reason.class);
+ reasons.add(TopologyMismatch.Reason.HOME_KEY);
+ }
+ Seekables<?, ?> updatedRanges = keysOrRanges.slice(t.ranges(),
Routables.Slice.Minimal);
+ if (!updatedRanges.containsAll(keysOrRanges))
Review Comment:
Could we use AbstractKeys#any here, or something similar so we don't have to
allocate a slice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
