You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "beobal (via GitHub)" <gi...@apache.org> on 2024/03/22 08:36:57 UTC
[PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
beobal opened a new pull request, #3196:
URL: https://github.com/apache/cassandra/pull/3196
When an instance moves from a transient to a full replica for a given range, it must begin acting as a full replica for writes before it does so for reads. Otherwise, consistency can be violated as data streamed to the instance early in the operation can be removed by cleanup if it occurs before the instance assumes responsibility for full writes. Also, coordinators will route read requests to instances which may not have received all preceding writes, causing unnecessary read repair or potentially inconsistent results.
The root cause of the flaky test failure which originally produced this issue was that in `TransientRangeMovementTest::testRemoveNode` cleanup happened to run on one instance before it had enacted the final step of the removal operation, leading it to remove more data than it should.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559437385
##########
test/distributed/org/apache/cassandra/distributed/test/TransientRangeMovement2Test.java:
##########
@@ -51,9 +59,30 @@ public void testMoveBackward() throws IOException, ExecutionException, Interrupt
.start()))
{
populate(cluster);
- cluster.get(3).nodetoolResult("move", "25").asserts().success();
+ // At the start, node1 is a TRANSIENT replica for (20,30] and FULL for (30, 40], (40,] & (,10]. When moving
+ // node3 to token 25, node1 becomes a FULL replica for (25, 40], effectively going from TRANSIENT to FULL
+ // for (25,30]. A T->F transition will always cause data for that range to be streamed to the transitioning
+ // node, which happens after StartMove and before MidMove. Running cleanup before node1 considers itself a
+ // FULL replica would remove any of the newly streamed data which is marked repaired. To avoid this, we
+ // ensure that any T->F transition is applied for writes as part of the StartMove. Have the CMS node (node1)
+ // pause before the MidMove step is committed, at which point we know that streaming has completed.
+ Callable<Epoch> pending = pauseBeforeCommit(cluster.get(1), (e) -> e instanceof PrepareMove.MidMove);
+ new Thread(() -> cluster.get(3).nodetoolResult("move", "25").asserts().success()).start();
Review Comment:
Would you mind to add a `join` after we have finished with this thread below?
##########
src/java/org/apache/cassandra/tcm/sequences/RemoveNodeStreams.java:
##########
@@ -131,33 +131,12 @@ private static MovementMap movementMap(InetAddressAndPort leaving, ClusterMetada
if (!replica.endpoint().equals(leaving) && !replica.endpoint().equals(newReplica.endpoint()))
candidateBuilder.add(replica, ReplicaCollection.Builder.Conflict.NONE);
});
- movements.putAll(newReplica, candidateBuilder.build(), ReplicaCollection.Builder.Conflict.NONE);
+ EndpointsForRange sources = candidateBuilder.build();
+ // log if newReplica is an existing transient replica moving to a full replica
Review Comment:
We can make a similar change in `UnbootstrapStreams` calculation of the movement map, change the mid check to
```
startWriteAdditions.get(removal.endpoint()).contains(removal.range(), true);
```
##########
src/java/org/apache/cassandra/tcm/sequences/RemoveNodeStreams.java:
##########
@@ -131,33 +131,12 @@ private static MovementMap movementMap(InetAddressAndPort leaving, ClusterMetada
if (!replica.endpoint().equals(leaving) && !replica.endpoint().equals(newReplica.endpoint()))
candidateBuilder.add(replica, ReplicaCollection.Builder.Conflict.NONE);
});
- movements.putAll(newReplica, candidateBuilder.build(), ReplicaCollection.Builder.Conflict.NONE);
+ EndpointsForRange sources = candidateBuilder.build();
+ // log if newReplica is an existing transient replica moving to a full replica
+ if (startWriteRemovals.get(newReplica.endpoint()).contains(newReplica.range(), false))
+ logger.debug("Streaming transient -> full conversion to {} from {}", newReplica.endpoint(), sources);
+ movements.putAll(newReplica, sources, ReplicaCollection.Builder.Conflict.NONE);
});
- // and check if any replicas went from transient -> full:
Review Comment:
it also happens that we do not need mid and finish delta for this method anymore. Should we remove them?
##########
src/java/org/apache/cassandra/tcm/transformations/PrepareMove.java:
##########
@@ -103,7 +104,9 @@ public Result execute(ClusterMetadata prev)
StartMove startMove = new StartMove(nodeId, transitionPlan.addToWrites(), lockKey);
MidMove midMove = new MidMove(nodeId, transitionPlan.moveReads(), lockKey);
FinishMove finishMove = new FinishMove(nodeId, tokens, transitionPlan.removeFromWrites(), lockKey);
-
+ Result res = PlacementTransitionPlan.assertPreExistingWriteReplica(prev.placements, transitionPlan);
Review Comment:
Do you folks think maybe having an Option here could be useful? Or some form of polymorphic result that would visually explain the significance of `null` equivalent?
##########
test/distributed/org/apache/cassandra/distributed/test/log/PlacementSimulator.java:
##########
@@ -985,20 +977,84 @@ public Diff<T> invert()
}
}
- public static String diffsToString(Map<Range, Diff<Node>> placements)
+ public static Diff<Replica> additionsAndTransientToFull(Diff<Replica> unfiltered)
+ {
+ if (unfiltered.additions.isEmpty())
+ return null;
+ List<Replica> additions = new ArrayList<>(unfiltered.additions.size());
+ List<Replica> removals = new ArrayList<>(unfiltered.additions.size());
+ for (Replica added : unfiltered.additions)
+ {
+ // Include any new FULL replicas here. If there exists the removal of a corresponding Transient
+ // replica (i.e. a switch from T -> F), add that too. We want T -> F transitions to happen early
+ // in a multi-step operation, at the same time as new write replicas are added.
+ if (added.isFull())
+ {
+ additions.add(added);
+ for (Replica removed : unfiltered.removals)
+ if (removed.node().equals(added.node()) && removed.isTransient())
+ removals.add(removed);
+ }
+ else
+ {
+ // Conversely, when a replica transitions from F -> T, it's enacted late in a multi-step operation.
+ // So only include TRANSIENT additions if there is no removal of a corresponding FULL replica.
+ boolean include = unfiltered.removals.stream()
+ .noneMatch(removed -> removed.node().equals(added.node())
+ && removed.isFull());
+ if (include)
+ additions.add(added);
+ }
+ }
+ return new Diff<>(additions, removals);
+ }
+
+ public static Diff<Replica> removalsAndFullToTransient(Diff<Replica> unfiltered)
+ {
+ if (unfiltered.removals.isEmpty())
+ return null;
+ List<Replica> additions = new ArrayList<>(unfiltered.removals.size());
+ List<Replica> removals = new ArrayList<>(unfiltered.removals.size());
+ for (Replica removed : unfiltered.removals)
+ {
+ // Include any new FULL replicas here. If there exists the removal of a corresponding Transient
+ // replica (i.e. a switch from T -> F), add that too. We want T -> F transitions to happen early
+ // in a multi-step operation, at the same time as new write replicas are added.
+ if (removed.isFull())
+ {
+ removals.add(removed);
+ for (Replica added : unfiltered.additions)
Review Comment:
Nit: I think same comment applies then to the above `additionsAndTransientToFull` code.
##########
test/harry/main/org/apache/cassandra/harry/sut/TokenPlacementModel.java:
##########
@@ -62,18 +75,34 @@ public ReplicatedRanges replicate(List<Node> nodes)
public abstract ReplicatedRanges replicate(Range[] ranges, List<Node> nodes);
}
+ public static class DCReplicas
+ {
+ public final int totalCount;
Review Comment:
Nit: should we try keeping API here consistent with RF?
```
public final int allReplicas;
public final int fullReplicas;
```
##########
test/distributed/org/apache/cassandra/distributed/test/log/PlacementSimulator.java:
##########
@@ -985,20 +977,84 @@ public Diff<T> invert()
}
}
- public static String diffsToString(Map<Range, Diff<Node>> placements)
+ public static Diff<Replica> additionsAndTransientToFull(Diff<Replica> unfiltered)
+ {
+ if (unfiltered.additions.isEmpty())
+ return null;
+ List<Replica> additions = new ArrayList<>(unfiltered.additions.size());
+ List<Replica> removals = new ArrayList<>(unfiltered.additions.size());
+ for (Replica added : unfiltered.additions)
+ {
+ // Include any new FULL replicas here. If there exists the removal of a corresponding Transient
+ // replica (i.e. a switch from T -> F), add that too. We want T -> F transitions to happen early
+ // in a multi-step operation, at the same time as new write replicas are added.
+ if (added.isFull())
+ {
+ additions.add(added);
+ for (Replica removed : unfiltered.removals)
Review Comment:
Nit: should use same style for filtering / checking removals in both branches?
I _think_ we can also break out from the loop here early if we did add a new entry to `removals`.
##########
test/distributed/org/apache/cassandra/distributed/test/TransientRangeMovement2Test.java:
##########
@@ -51,9 +59,30 @@ public void testMoveBackward() throws IOException, ExecutionException, Interrupt
.start()))
{
populate(cluster);
- cluster.get(3).nodetoolResult("move", "25").asserts().success();
+ // At the start, node1 is a TRANSIENT replica for (20,30] and FULL for (30, 40], (40,] & (,10]. When moving
+ // node3 to token 25, node1 becomes a FULL replica for (25, 40], effectively going from TRANSIENT to FULL
+ // for (25,30]. A T->F transition will always cause data for that range to be streamed to the transitioning
+ // node, which happens after StartMove and before MidMove. Running cleanup before node1 considers itself a
+ // FULL replica would remove any of the newly streamed data which is marked repaired. To avoid this, we
+ // ensure that any T->F transition is applied for writes as part of the StartMove. Have the CMS node (node1)
+ // pause before the MidMove step is committed, at which point we know that streaming has completed.
+ Callable<Epoch> pending = pauseBeforeCommit(cluster.get(1), (e) -> e instanceof PrepareMove.MidMove);
+ new Thread(() -> cluster.get(3).nodetoolResult("move", "25").asserts().success()).start();
+
+ // Before allowing node1 to proceed with committing the MidMove, instruct it to pause node1 before it
Review Comment:
nit: This line reads a bit tautological, would maybe something like "To gate/prevent node1 from proceeding with committing MidMove, instruct it to pause before enacting it" ?
##########
src/java/org/apache/cassandra/tcm/transformations/PrepareJoin.java:
##########
@@ -156,6 +153,12 @@ public Result execute(ClusterMetadata prev)
transitionPlan.toSplit,
startJoin, midJoin, finishJoin,
joinTokenRing, streamData);
+ if (!(this instanceof UnsafeJoin) && !prev.tokenMap.isEmpty())
Review Comment:
Would you be OK to move this logic to an instance method, and have implemenation in PrepareJoin, and override it in UnsafeJoin? It would be great if we could keep it polymorphic.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559717234
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementForRange.java:
##########
@@ -307,17 +307,51 @@ public Builder withReplicaGroup(VersionedEndpoints.ForRange replicas)
(t, v) -> {
EndpointsForRange old = v.get();
return VersionedEndpoints.forRange(Epoch.max(v.lastModified(), replicas.lastModified()),
- replicas.get()
- .newBuilder(replicas.size() + old.size())
- .addAll(old)
- .addAll(replicas.get(), ReplicaCollection.Builder.Conflict.ALL)
- .build());
+ combine(old, replicas.get()));
});
if (group == null)
replicaGroups.put(replicas.range(), replicas);
return this;
}
+ /**
+ * Combine two replica groups, assuming one is the current group and the other is the proposed.
+ * During range movements this is used when calculating the maximal placement, which combines the current and
+ * future replica groups. This special cases the merging of two replica groups to make sure that when a replica
+ * moves from transient to full, it starts to act as a FULL write replica as early as possible.
+ *
+ * Where an endpoint is present in both groups, prefer the proposed iff it is a FULL replica. During a
+ * multi-step operation (join/leave/move), we want any change from transient to full to happen as early
+ * as possible so that a replica being whose ownership is modified in this way becomes FULL for writes before it
Review Comment:
think its just that extra "being" which makes this confusing, fixed
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1555957713
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementForRange.java:
##########
@@ -307,17 +307,51 @@ public Builder withReplicaGroup(VersionedEndpoints.ForRange replicas)
(t, v) -> {
EndpointsForRange old = v.get();
return VersionedEndpoints.forRange(Epoch.max(v.lastModified(), replicas.lastModified()),
- replicas.get()
- .newBuilder(replicas.size() + old.size())
- .addAll(old)
- .addAll(replicas.get(), ReplicaCollection.Builder.Conflict.ALL)
- .build());
+ combine(old, replicas.get()));
});
if (group == null)
replicaGroups.put(replicas.range(), replicas);
return this;
}
+ /**
+ * Combine two replica groups, assuming one is the current group and the other is the proposed.
+ * During range movements this is used when calculating the maximal placement, which combines the current and
+ * future replica groups. This special cases the merging of two replica groups to make sure that when a replica
+ * moves from transient to full, it starts to act as a FULL write replica as early as possible.
+ *
+ * Where an endpoint is present in both groups, prefer the proposed iff it is a FULL replica. During a
+ * multi-step operation (join/leave/move), we want any change from transient to full to happen as early
+ * as possible so that a replica being whose ownership is modified in this way becomes FULL for writes before it
Review Comment:
nit: Could you rephrase this part a bit?
> a replica being whose ownership is modified in this way becomes FULL for writes before it
I am a bit thrown off by replica being part.
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementTransitionPlan.java:
##########
@@ -125,4 +144,78 @@ public String toString()
", compiled=" + (addToWrites == null) +
'}';
}
+
+
+ /**
+ * Makes sure that a newly added read replica for a range already exists as a write replica
+ *
+ * We should never add both read & write replicas for the same range at the same time (or read replica before write)
+ *
+ * Also makes sure that we don't add a full read replica while the same write replica is only transient - we should
+ * always make the write replica full before adding the read replica.
+ *
+ * We split and merge ranges, so in the previous placements we could have full write replicas (a, b], (b, c], but then
+ * add a full read replica (a, c].
+ *
+ * @return null if everything is good, otherwise a Transformation.Result rejection containing information about the bad replica
+ */
+ @Nullable
+ public static Transformation.Result assertPreExistingWriteReplica(DataPlacements placements, PlacementTransitionPlan transitionPlan)
+ {
+ return assertPreExistingWriteReplica(placements,
+ transitionPlan.toSplit,
+ transitionPlan.addToWrites(),
+ transitionPlan.moveReads(),
+ transitionPlan.removeFromWrites());
+ }
+
+ @Nullable
+ public static Transformation.Result assertPreExistingWriteReplica(DataPlacements placements, PlacementDeltas ... deltasInOrder)
+ {
+ for (PlacementDeltas deltas : deltasInOrder)
+ {
+ for (Map.Entry<ReplicationParams, PlacementDeltas.PlacementDelta> entry : deltas)
+ {
+ ReplicationParams params = entry.getKey();
+ PlacementDeltas.PlacementDelta delta = entry.getValue();
+ for (Map.Entry<InetAddressAndPort, RangesAtEndpoint> addedRead : delta.reads.additions.entrySet())
+ {
+ RangesAtEndpoint addedReadReplicas = addedRead.getValue();
+ RangesAtEndpoint existingWriteReplicas = placements.get(params).writes.byEndpoint().get(addedRead.getKey());
+ // we're adding read replicas - they should always exist as write replicas before doing that
+ // BUT we split and merge ranges so we need to check containment both ways
+ for (Replica newReadReplica : addedReadReplicas)
+ {
+ if (existingWriteReplicas.contains(newReadReplica))
+ continue;
+ boolean contained = false;
+ Set<Range<Token>> intersectingRanges = new HashSet<>();
+ for (Replica writeReplica : existingWriteReplicas)
+ {
+ if (writeReplica.isFull() == newReadReplica.isFull() || writeReplica.isFull() && newReadReplica.isTransient())
Review Comment:
nit: would you mind adding brackets here to make operator order more explicit here?
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementForRange.java:
##########
@@ -307,17 +307,51 @@ public Builder withReplicaGroup(VersionedEndpoints.ForRange replicas)
(t, v) -> {
EndpointsForRange old = v.get();
return VersionedEndpoints.forRange(Epoch.max(v.lastModified(), replicas.lastModified()),
- replicas.get()
- .newBuilder(replicas.size() + old.size())
- .addAll(old)
- .addAll(replicas.get(), ReplicaCollection.Builder.Conflict.ALL)
- .build());
+ combine(old, replicas.get()));
});
if (group == null)
replicaGroups.put(replicas.range(), replicas);
return this;
}
+ /**
+ * Combine two replica groups, assuming one is the current group and the other is the proposed.
+ * During range movements this is used when calculating the maximal placement, which combines the current and
+ * future replica groups. This special cases the merging of two replica groups to make sure that when a replica
+ * moves from transient to full, it starts to act as a FULL write replica as early as possible.
+ *
+ * Where an endpoint is present in both groups, prefer the proposed iff it is a FULL replica. During a
+ * multi-step operation (join/leave/move), we want any change from transient to full to happen as early
+ * as possible so that a replica being whose ownership is modified in this way becomes FULL for writes before it
+ * becomes FULL for reads. This works as additions to write replica groups are applied before any other
+ * placement changes (i.e. in START_[JOIN|LEAVE|MOVE]).
+ *
+ * @param prev Initial set of replicas for a given range
+ * @param next Proposed set of replicas for the same range.
+ * @return The union of the two groups
+ */
+ private EndpointsForRange combine(EndpointsForRange prev, EndpointsForRange next)
+ {
+ Map<InetAddressAndPort, Replica> e1 = prev.byEndpoint();
+ Map<InetAddressAndPort, Replica> e2 = next.byEndpoint();
+ EndpointsForRange.Builder combined = prev.newBuilder(prev.size() + next.size());
+ e1.forEach((e, r1) -> {
+ Replica r2 = e2.get(e);
+ if (null == r2) // not present in next
+ combined.add(r1);
+ else if (r2.isFull()) // prefer replica from next, if it is moving from transient to full
Review Comment:
nit: could probably short-circuit this with
```
if (r2 != null && r2.isFull)
combined.add(r2);
else
combined.add(r1);
```
If you deliberately have split these, I do nit mind leaving as-is.
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementForRange.java:
##########
@@ -307,17 +307,51 @@ public Builder withReplicaGroup(VersionedEndpoints.ForRange replicas)
(t, v) -> {
EndpointsForRange old = v.get();
return VersionedEndpoints.forRange(Epoch.max(v.lastModified(), replicas.lastModified()),
- replicas.get()
- .newBuilder(replicas.size() + old.size())
- .addAll(old)
- .addAll(replicas.get(), ReplicaCollection.Builder.Conflict.ALL)
- .build());
+ combine(old, replicas.get()));
});
if (group == null)
replicaGroups.put(replicas.range(), replicas);
return this;
}
+ /**
+ * Combine two replica groups, assuming one is the current group and the other is the proposed.
+ * During range movements this is used when calculating the maximal placement, which combines the current and
+ * future replica groups. This special cases the merging of two replica groups to make sure that when a replica
+ * moves from transient to full, it starts to act as a FULL write replica as early as possible.
+ *
+ * Where an endpoint is present in both groups, prefer the proposed iff it is a FULL replica. During a
+ * multi-step operation (join/leave/move), we want any change from transient to full to happen as early
+ * as possible so that a replica being whose ownership is modified in this way becomes FULL for writes before it
+ * becomes FULL for reads. This works as additions to write replica groups are applied before any other
+ * placement changes (i.e. in START_[JOIN|LEAVE|MOVE]).
+ *
+ * @param prev Initial set of replicas for a given range
+ * @param next Proposed set of replicas for the same range.
+ * @return The union of the two groups
+ */
+ private EndpointsForRange combine(EndpointsForRange prev, EndpointsForRange next)
+ {
+ Map<InetAddressAndPort, Replica> e1 = prev.byEndpoint();
+ Map<InetAddressAndPort, Replica> e2 = next.byEndpoint();
+ EndpointsForRange.Builder combined = prev.newBuilder(prev.size() + next.size());
+ e1.forEach((e, r1) -> {
+ Replica r2 = e2.get(e);
+ if (null == r2) // not present in next
+ combined.add(r1);
+ else if (r2.isFull()) // prefer replica from next, if it is moving from transient to full
+ combined.add(r2);
+ else
+ combined.add(r1); // replica is moving from full to transient, or staying the same
+ });
+ // any new replicas not in prev
+ e2.forEach((e, r2) -> {
Review Comment:
Would it make sense to construct a set to iterate over in the above loop?
##########
test/harry/main/org/apache/cassandra/harry/sut/TokenPlacementModel.java:
##########
@@ -199,41 +228,28 @@ public static List<Node> peerStateToNodes(Object[][] resultset)
public static class NtsReplicationFactor extends ReplicationFactor
{
- private final Lookup lookup = new DefaultLookup();
Review Comment:
oh thank you for removing this! This was an artifact of integrating with a simulator, but it never was actually used in this context.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559809923
##########
src/java/org/apache/cassandra/tcm/transformations/PrepareMove.java:
##########
@@ -103,7 +104,9 @@ public Result execute(ClusterMetadata prev)
StartMove startMove = new StartMove(nodeId, transitionPlan.addToWrites(), lockKey);
MidMove midMove = new MidMove(nodeId, transitionPlan.moveReads(), lockKey);
FinishMove finishMove = new FinishMove(nodeId, tokens, transitionPlan.removeFromWrites(), lockKey);
-
+ Result res = PlacementTransitionPlan.assertPreExistingWriteReplica(prev.placements, transitionPlan);
Review Comment:
made `assertPreExistingWriteReplica` throw `RejectedTransformationException` instead
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559816528
##########
test/harry/main/org/apache/cassandra/harry/sut/TokenPlacementModel.java:
##########
@@ -62,18 +75,34 @@ public ReplicatedRanges replicate(List<Node> nodes)
public abstract ReplicatedRanges replicate(Range[] ranges, List<Node> nodes);
}
+ public static class DCReplicas
+ {
+ public final int totalCount;
Review Comment:
think the naming is clear enough that this won't introduce any issues
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559787749
##########
src/java/org/apache/cassandra/tcm/sequences/RemoveNodeStreams.java:
##########
@@ -131,33 +131,12 @@ private static MovementMap movementMap(InetAddressAndPort leaving, ClusterMetada
if (!replica.endpoint().equals(leaving) && !replica.endpoint().equals(newReplica.endpoint()))
candidateBuilder.add(replica, ReplicaCollection.Builder.Conflict.NONE);
});
- movements.putAll(newReplica, candidateBuilder.build(), ReplicaCollection.Builder.Conflict.NONE);
+ EndpointsForRange sources = candidateBuilder.build();
+ // log if newReplica is an existing transient replica moving to a full replica
Review Comment:
yep, almost - this became
```
if (startWriteRemovals.get(newReplica.endpoint()).contains(newReplica.range(), false))
logger.debug("Streaming transient -> full conversion to {} from {}", newReplica, oldReplicas.get(newReplica.range()));
movements.put(oldReplicas.get(newReplica.range()), newReplica);
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1560745961
##########
test/distributed/org/apache/cassandra/distributed/test/log/PlacementSimulator.java:
##########
@@ -985,20 +977,84 @@ public Diff<T> invert()
}
}
- public static String diffsToString(Map<Range, Diff<Node>> placements)
+ public static Diff<Replica> additionsAndTransientToFull(Diff<Replica> unfiltered)
+ {
+ if (unfiltered.additions.isEmpty())
+ return null;
+ List<Replica> additions = new ArrayList<>(unfiltered.additions.size());
+ List<Replica> removals = new ArrayList<>(unfiltered.additions.size());
+ for (Replica added : unfiltered.additions)
+ {
+ // Include any new FULL replicas here. If there exists the removal of a corresponding Transient
+ // replica (i.e. a switch from T -> F), add that too. We want T -> F transitions to happen early
+ // in a multi-step operation, at the same time as new write replicas are added.
+ if (added.isFull())
+ {
+ additions.add(added);
+ for (Replica removed : unfiltered.removals)
Review Comment:
I thought maybe something like this:
```
if (added.isFull())
{
additions.add(added);
Optional<Replica> removed = unfiltered.removals.stream()
.filter(r -> r.isTransient() && r.node().equals(added.node()))
.findFirst();
if (removed.isPresent())
removals.add(removed.get());
}
else
{
// Conversely, when a replica transitions from F -> T, it's enacted late in a multi-step operation.
// So only include TRANSIENT additions if there is no removal of a corresponding FULL replica.
boolean include = unfiltered.removals.stream()
.noneMatch(removed -> removed.node().equals(added.node())
&& removed.isFull());
if (include)
additions.add(added);
}
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559819996
##########
test/distributed/org/apache/cassandra/distributed/test/log/PlacementSimulator.java:
##########
@@ -985,20 +977,84 @@ public Diff<T> invert()
}
}
- public static String diffsToString(Map<Range, Diff<Node>> placements)
+ public static Diff<Replica> additionsAndTransientToFull(Diff<Replica> unfiltered)
+ {
+ if (unfiltered.additions.isEmpty())
+ return null;
+ List<Replica> additions = new ArrayList<>(unfiltered.additions.size());
+ List<Replica> removals = new ArrayList<>(unfiltered.additions.size());
+ for (Replica added : unfiltered.additions)
+ {
+ // Include any new FULL replicas here. If there exists the removal of a corresponding Transient
+ // replica (i.e. a switch from T -> F), add that too. We want T -> F transitions to happen early
+ // in a multi-step operation, at the same time as new write replicas are added.
+ if (added.isFull())
+ {
+ additions.add(added);
+ for (Replica removed : unfiltered.removals)
Review Comment:
what did you have in mind here?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "krummas (via GitHub)" <gi...@apache.org>.
krummas commented on code in PR #3196:
URL: https://github.com/apache/cassandra/pull/3196#discussion_r1559744473
##########
src/java/org/apache/cassandra/tcm/ownership/PlacementForRange.java:
##########
@@ -307,17 +307,51 @@ public Builder withReplicaGroup(VersionedEndpoints.ForRange replicas)
(t, v) -> {
EndpointsForRange old = v.get();
return VersionedEndpoints.forRange(Epoch.max(v.lastModified(), replicas.lastModified()),
- replicas.get()
- .newBuilder(replicas.size() + old.size())
- .addAll(old)
- .addAll(replicas.get(), ReplicaCollection.Builder.Conflict.ALL)
- .build());
+ combine(old, replicas.get()));
});
if (group == null)
replicaGroups.put(replicas.range(), replicas);
return this;
}
+ /**
+ * Combine two replica groups, assuming one is the current group and the other is the proposed.
+ * During range movements this is used when calculating the maximal placement, which combines the current and
+ * future replica groups. This special cases the merging of two replica groups to make sure that when a replica
+ * moves from transient to full, it starts to act as a FULL write replica as early as possible.
+ *
+ * Where an endpoint is present in both groups, prefer the proposed iff it is a FULL replica. During a
+ * multi-step operation (join/leave/move), we want any change from transient to full to happen as early
+ * as possible so that a replica being whose ownership is modified in this way becomes FULL for writes before it
+ * becomes FULL for reads. This works as additions to write replica groups are applied before any other
+ * placement changes (i.e. in START_[JOIN|LEAVE|MOVE]).
+ *
+ * @param prev Initial set of replicas for a given range
+ * @param next Proposed set of replicas for the same range.
+ * @return The union of the two groups
+ */
+ private EndpointsForRange combine(EndpointsForRange prev, EndpointsForRange next)
+ {
+ Map<InetAddressAndPort, Replica> e1 = prev.byEndpoint();
+ Map<InetAddressAndPort, Replica> e2 = next.byEndpoint();
+ EndpointsForRange.Builder combined = prev.newBuilder(prev.size() + next.size());
+ e1.forEach((e, r1) -> {
+ Replica r2 = e2.get(e);
+ if (null == r2) // not present in next
+ combined.add(r1);
+ else if (r2.isFull()) // prefer replica from next, if it is moving from transient to full
Review Comment:
I think I prefer the split version for the comments per-case
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3196: CASSANDRA-19344 Range movements involving transient replicas must safely enact changes to read and write replica sets
URL: https://github.com/apache/cassandra/pull/3196
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org