Claudenw commented on code in PR #2259:
URL: https://github.com/apache/cassandra/pull/2259#discussion_r1156889191
##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,142 @@ private static Event writeOnly(int pk)
return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
}
- private void fromLog(String log)
+ private interface Operation {}
+ private static class Read implements Operation
{
- IntSet pks = new IntHashSet();
- class Read
- {
- final int pk, id, count;
- final int[] seq;
+ final int pk, id, count;
+ final int[] seq;
- Read(int pk, int id, int count, 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;
}
- class Write
+ }
+ private static class Write implements Operation
+ {
+ final int pk, id;
+ final boolean success;
+
+ Write(int pk, int id, boolean success)
{
- final int pk, id;
- final boolean success;
+ this.pk = pk;
+ this.id = id;
+ this.success = success;
+ }
+ }
- Write(int pk, int id, boolean success)
- {
- this.pk = pk;
- this.id = id;
- this.success = success;
- }
+ private static class Witness
+ {
+ final int start, end;
+ final List<Operation> actions = new ArrayList<>();
+
+ Witness(int start, int end)
+ {
+ this.start = start;
+ this.end = end;
}
- class Witness
+
+ void read(int pk, int id, int count, int[] seq)
{
- final int start, end;
- final List<Object> actions = new ArrayList<>();
+ actions.add(new Read(pk, id, count, seq));
+ }
- Witness(int start, int end)
- {
- this.start = start;
- this.end = end;
- }
+ void write(int pk, int id, boolean success)
+ {
+ actions.add(new Write(pk, id, success));
+ }
- void read(int pk, int id, int count, int[] seq)
+ void process(HistoryValidator validator)
+ {
+ try (HistoryValidator.Checker check = validator.witness(start,
end))
{
- actions.add(new Read(pk, id, count, seq));
+ for (Object a : actions)
+ {
+ if (a instanceof Read)
+ {
+ Read read = (Read) a;
+ check.read(read.pk, read.id, read.count, read.seq);
+ }
+ else
+ {
+ Write write = (Write) a;
+ check.write(write.pk, write.id, write.success);
+ }
+ }
}
+ }
- void write(int pk, int id, boolean success)
+ IntSet pks()
+ {
+ IntSet pks = new IntHashSet();
+ for (Operation action : actions)
{
- actions.add(new Write(pk, id, success));
+ if (action instanceof Read)
+ {
+ pks.add(((Read) action).pk);
+ }
+ else if (action instanceof Write)
+ {
+ pks.add(((Write) action).pk);
+ }
+ else
+ {
+ throw new IllegalStateException("Unknown type: " +
action.getClass());
+ }
}
+ return pks;
+ }
- void process(HistoryValidator validator)
+ @Override
+ public String toString()
+ {
+ StringBuilder sb = new StringBuilder();
+ sb.append("Witness(start=").append(start).append(",
end=").append(end).append(")\n");
+ for (Object a : actions)
{
- try (HistoryValidator.Checker check = validator.witness(start,
end))
+ if (a instanceof Read)
{
- for (Object a : actions)
- {
- if (a instanceof Read)
- {
- Read read = (Read) a;
- check.read(read.pk, read.id, read.count, read.seq);
- }
- else
- {
- Write write = (Write) a;
- check.write(write.pk, write.id, write.success);
- }
- }
+ Read read = (Read) a;
+ sb.append("\tread(pk=").append(read.pk).append(",
id=").append(read.id).append(", count=").append(read.count).append(",
seq=").append(Arrays.toString(read.seq)).append(")\n");
+ }
+ else if (a instanceof Write)
+ {
+ Write write = (Write) a;
+ sb.append("\twrite(pk=").append(write.pk).append(",
id=").append(write.id).append(", success=").append(write.success).append(")\n");
+ }
+ else
+ {
+ throw new AssertionError("Unexpected type: " +
a.getClass());
}
}
Review Comment:
As above, shouldn't the `Operation` implementations `Read` and `Write`
override `toString()`? Then the method could simply be called here on each
action.
##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,142 @@ private static Event writeOnly(int pk)
return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
}
- private void fromLog(String log)
+ private interface Operation {}
+ private static class Read implements Operation
{
- IntSet pks = new IntHashSet();
- class Read
- {
- final int pk, id, count;
- final int[] seq;
+ final int pk, id, count;
+ final int[] seq;
- Read(int pk, int id, int count, 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;
}
- class Write
+ }
+ private static class Write implements Operation
+ {
+ final int pk, id;
+ final boolean success;
+
+ Write(int pk, int id, boolean success)
{
- final int pk, id;
- final boolean success;
+ this.pk = pk;
+ this.id = id;
+ this.success = success;
+ }
+ }
- Write(int pk, int id, boolean success)
- {
- this.pk = pk;
- this.id = id;
- this.success = success;
- }
+ private static class Witness
+ {
+ final int start, end;
+ final List<Operation> actions = new ArrayList<>();
+
+ Witness(int start, int end)
+ {
+ this.start = start;
+ this.end = end;
}
- class Witness
+
+ void read(int pk, int id, int count, int[] seq)
{
- final int start, end;
- final List<Object> actions = new ArrayList<>();
+ actions.add(new Read(pk, id, count, seq));
+ }
- Witness(int start, int end)
- {
- this.start = start;
- this.end = end;
- }
+ void write(int pk, int id, boolean success)
+ {
+ actions.add(new Write(pk, id, success));
+ }
- void read(int pk, int id, int count, int[] seq)
+ void process(HistoryValidator validator)
+ {
+ try (HistoryValidator.Checker check = validator.witness(start,
end))
{
- actions.add(new Read(pk, id, count, seq));
+ for (Object a : actions)
+ {
+ if (a instanceof Read)
+ {
+ Read read = (Read) a;
+ check.read(read.pk, read.id, read.count, read.seq);
+ }
+ else
+ {
+ Write write = (Write) a;
+ check.write(write.pk, write.id, write.success);
+ }
+ }
Review Comment:
Since `actions` is a List<Operation> would it make sense to add a
`validate(HistoryValidator.Checker)` method to `Operaion` and simply call it
here passing the `check` variable as an argument?
##########
test/simulator/test/org/apache/cassandra/simulator/paxos/HistoryValidatorTest.java:
##########
@@ -356,79 +383,142 @@ private static Event writeOnly(int pk)
return new Event(EnumSet.of(Event.Type.WRITE), pk, null);
}
- private void fromLog(String log)
+ private interface Operation {}
+ private static class Read implements Operation
{
- IntSet pks = new IntHashSet();
- class Read
- {
- final int pk, id, count;
- final int[] seq;
+ final int pk, id, count;
+ final int[] seq;
- Read(int pk, int id, int count, 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;
}
- class Write
+ }
+ private static class Write implements Operation
+ {
+ final int pk, id;
+ final boolean success;
+
+ Write(int pk, int id, boolean success)
{
- final int pk, id;
- final boolean success;
+ this.pk = pk;
+ this.id = id;
+ this.success = success;
+ }
+ }
- Write(int pk, int id, boolean success)
- {
- this.pk = pk;
- this.id = id;
- this.success = success;
- }
+ private static class Witness
+ {
+ final int start, end;
+ final List<Operation> actions = new ArrayList<>();
+
+ Witness(int start, int end)
+ {
+ this.start = start;
+ this.end = end;
}
- class Witness
+
+ void read(int pk, int id, int count, int[] seq)
{
- final int start, end;
- final List<Object> actions = new ArrayList<>();
+ actions.add(new Read(pk, id, count, seq));
+ }
- Witness(int start, int end)
- {
- this.start = start;
- this.end = end;
- }
+ void write(int pk, int id, boolean success)
+ {
+ actions.add(new Write(pk, id, success));
+ }
- void read(int pk, int id, int count, int[] seq)
+ void process(HistoryValidator validator)
+ {
+ try (HistoryValidator.Checker check = validator.witness(start,
end))
{
- actions.add(new Read(pk, id, count, seq));
+ for (Object a : actions)
+ {
+ if (a instanceof Read)
+ {
+ Read read = (Read) a;
+ check.read(read.pk, read.id, read.count, read.seq);
+ }
+ else
+ {
+ Write write = (Write) a;
+ check.write(write.pk, write.id, write.success);
+ }
+ }
}
+ }
- void write(int pk, int id, boolean success)
+ IntSet pks()
+ {
+ IntSet pks = new IntHashSet();
+ for (Operation action : actions)
{
- actions.add(new Write(pk, id, success));
+ if (action instanceof Read)
+ {
+ pks.add(((Read) action).pk);
+ }
+ else if (action instanceof Write)
+ {
+ pks.add(((Write) action).pk);
+ }
+ else
+ {
+ throw new IllegalStateException("Unknown type: " +
action.getClass());
+ }
}
+ return pks;
Review Comment:
Like the previous comment, would it not make sense for Operation to have a
`getPK()` method to so that the set of PK can be generated without `instanceof`
checks?
--
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]
