[GitHub] [cassandra] bdeggleston commented on a diff in pull request #1948: Feedback/cassandra 17103

[GitBox <gi...@apache.org>]

bdeggleston commented on code in PR #1948:
URL: https://github.com/apache/cassandra/pull/1948#discussion_r1006286711


##########
src/java/org/apache/cassandra/service/accord/serializers/AcceptSerializers.java:
##########
@@ -104,22 +104,29 @@ public long serializedSize(Accept.Invalidate invalidate, int version)
         public void serialize(AcceptOk acceptOk, DataOutputPlus out, int version) throws IOException
         {
             CommandSerializers.txnId.serialize(acceptOk.txnId, out, version);
-            CommandSerializers.deps.serialize(acceptOk.deps, out, version);
+            boolean hasDeps = acceptOk.deps != null;

Review Comment:
   can we use NullableSerializer here?



##########
src/java/org/apache/cassandra/service/accord/AccordKeyspace.java:
##########
@@ -115,12 +115,12 @@
     private static final Logger logger = LoggerFactory.getLogger(AccordKeyspace.class);
 
     public static final String COMMANDS = "commands";
-    public static final String COMMAND_SERIES = "command_series";
     public static final String COMMANDS_FOR_KEY = "commands_for_key";
 
     private static final String TIMESTAMP_TUPLE = "tuple<bigint, bigint, int, bigint>";
     private static final TupleType TIMESTAMP_TYPE = new TupleType(Lists.newArrayList(LongType.instance, LongType.instance, Int32Type.instance, LongType.instance));
     private static final String KEY_TUPLE = "tuple<uuid, blob>";
+    private static final int CURRENT_VERSION = 1;

Review Comment:
   this should be `MessagingService.current_version`



##########
src/java/org/apache/cassandra/service/accord/async/AsyncWriter.java:
##########
@@ -238,7 +240,7 @@ private void denormalize(AccordCommand command, AsyncContext context, Object cal
         }
 
         // There won't be a txn to denormalize against until the command has been preaccepted
-        if (command.status().hasBeen(Status.PreAccepted) && AccordPartialCommand.WithDeps.serializer.needsUpdate(command))
+        if (command.status().hasBeen(Status.PreAccepted) && AccordPartialCommand.WithDeps.serializer.needsUpdate(command) && !(command.txn() == null && command.status().isInvalidated()))

Review Comment:
   Not denormalizing an invalidated command may leave it in the uncommitted set, which could lead to incorrect deps being reported. That said, cfk denormalization may no longer be needed, and I'm going to try removing it in another branch, so hang tight



##########
src/java/org/apache/cassandra/service/accord/AccordKeyspace.java:
##########
@@ -433,31 +424,27 @@ public static Mutation getCommandMutation(AccordCommand command, long timestampM
             Row.Builder builder = BTreeRow.unsortedBuilder();
             builder.newRow(Clustering.EMPTY);
             int nowInSeconds = (int) TimeUnit.MICROSECONDS.toSeconds(timestampMicros);
-            int version = MessagingService.current_version;
-            ByteBuffer versionBytes = accessor.valueOf(version);
+
 
             if (command.status.hasModifications())
                 builder.addCell(live(CommandsColumns.status, timestampMicros, accessor.valueOf(command.status.get().ordinal())));
 
             if (command.homeKey.hasModifications())
             {

Review Comment:
   we can drop the braces for the blocks that have become single lines



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