Claudenw commented on code in PR #2259:
URL: https://github.com/apache/cassandra/pull/2259#discussion_r1158103859


##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,146 @@ private static Event writeOnly(int pk)
         return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
     }
 
-    private void fromLog(String log)
+    private interface Operation
     {
-        IntSet pks = new IntHashSet();
-        class Read
+        int pk();
+        void check(HistoryValidator.Checker check);
+        void toString(StringBuilder sb);

Review Comment:
   This name conflicts semantically with the `Object.toString()`  semantically 
this is probably better named `addString()`  or `appendString()`



##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,146 @@ private static Event writeOnly(int pk)
         return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
     }
 
-    private void fromLog(String log)
+    private interface Operation
     {
-        IntSet pks = new IntHashSet();
-        class Read
+        int pk();
+        void check(HistoryValidator.Checker check);
+        void toString(StringBuilder sb);
+    }
+
+    private static class Read implements Operation
+    {
+        final int pk, id, count;
+        final int[] seq;
+
+        Read(int pk, int id, int count, int[] seq)
         {
-            final int pk, id, count;
-            final int[] seq;
+            this.pk = pk;
+            this.id = id;
+            this.count = count;
+            this.seq = seq;
+        }
 
-            Read(int pk, int id, int count, int[] seq)
-            {
-                this.pk = pk;
-                this.id = id;
-                this.count = count;
-                this.seq = seq;
-            }
+        @Override
+        public int pk()
+        {
+            return pk;
         }
-        class Write
+
+        @Override
+        public void check(HistoryValidator.Checker check)
         {
-            final int pk, id;
-            final boolean success;
+            check.read(pk, id, count, seq);
+        }
 
-            Write(int pk, int id, boolean success)
-            {
-                this.pk = pk;
-                this.id = id;
-                this.success = success;
-            }
+        @Override
+        public void toString(StringBuilder sb)
+        {
+            sb.append("read(pk=").append(pk).append(", 
id=").append(id).append(", count=").append(count).append(", 
seq=").append(Arrays.toString(seq)).append(")\n");
         }
-        class Witness
+    }
+
+    private static class Write implements Operation
+    {
+        final int pk, id;
+        final boolean success;
+
+        Write(int pk, int id, boolean success)
         {
-            final int start, end;
-            final List<Object> actions = new ArrayList<>();
+            this.pk = pk;
+            this.id = id;
+            this.success = success;
+        }
 
-            Witness(int start, int end)
-            {
-                this.start = start;
-                this.end = end;
-            }
+        @Override
+        public int pk()
+        {
+            return pk;
+        }
 
-            void read(int pk, int id, int count, int[] seq)
-            {
-                actions.add(new Read(pk, id, count, seq));
-            }
+        @Override
+        public void check(HistoryValidator.Checker check)
+        {
+            check.write(pk, id, success);
+        }
 
-            void write(int pk, int id, boolean success)
-            {
-                actions.add(new Write(pk, id, success));
-            }
+        @Override
+        public void toString(StringBuilder sb)
+        {
+            sb.append("write(pk=").append(pk).append(", 
id=").append(id).append(", success=").append(success).append(")\n");
+        }
+    }
+
+    private static class Witness
+    {
+        final int start, end;
+        final List<Operation> actions = new ArrayList<>();

Review Comment:
   Is there any reason not to make the variables private?



##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,146 @@ private static Event writeOnly(int pk)
         return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
     }
 
-    private void fromLog(String log)
+    private interface Operation

Review Comment:
   If `Operation` was an abstract class rather than an interface you could 
implement pk() directly in the class and not repeat it in the subclasses.  I 
note that ID is also used in every instance of operation. 
   
   Do you expect other implementations of Operation that do not have the `pk` 
or `id` specified in the constructor?
   
   Is there any reason not to make the variable private?



##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,146 @@ private static Event writeOnly(int pk)
         return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
     }
 
-    private void fromLog(String log)
+    private interface Operation
     {
-        IntSet pks = new IntHashSet();
-        class Read
+        int pk();
+        void check(HistoryValidator.Checker check);
+        void toString(StringBuilder sb);
+    }
+
+    private static class Read implements Operation
+    {
+        final int pk, id, count;
+        final int[] seq;

Review Comment:
   Is there any reason not to make the variable private?



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

Reply via email to