dcapwell commented on code in PR #2804:
URL: https://github.com/apache/cassandra/pull/2804#discussion_r1367509230
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -910,7 +910,11 @@ public static void setClientMode(boolean clientMode)
public volatile Map<StartupCheckType, Map<String, Object>> startup_checks
= new HashMap<>();
public volatile DurationSpec.LongNanosecondsBound repair_state_expires =
new DurationSpec.LongNanosecondsBound("3d");
- public volatile int repair_state_size = 100_000;
+
+ // Only one of repair_state_size and repair_state_heap_size should be set
+ @Deprecated
Review Comment:
can you add a `since`? I think we are trying to do that now...
##########
src/java/org/apache/cassandra/repair/state/ParticipateState.java:
##########
@@ -53,14 +61,54 @@ public enum RegisterStatus
public final Phase phase = new Phase();
+ public final ConcurrentMap<RepairJobDesc, Job> jobs = new
ConcurrentHashMap<>();
+
+ private static final long EMPTY_SIZE = ObjectSizes.measure(new
ParticipateState(SharedContext.Global.instance.clock(), null, new
PrepareMessage(nextTimeUUID(), Collections.emptyList(),
Collections.emptyList(), false, 0L, false, PreviewKind.NONE)));
Review Comment:
should define the static as the first set of variables
##########
src/java/org/apache/cassandra/repair/state/ParticipateState.java:
##########
@@ -148,16 +200,43 @@ public void accept()
}
}
- public static class Job extends AbstractState<Job.State, RepairJobDesc>
+ public static class Job extends AbstractState<Job.State, RepairJobDesc>
implements WeightedHierarchy.InternalNode
{
public enum State { ACCEPT, SNAPSHOT, VALIDATION, SYNC }
+ private final ParticipateState participateState;
private final AtomicReference<ValidationState> validation = new
AtomicReference<>(null);
private final ConcurrentMap<SyncState.Id, SyncState> syncs = new
ConcurrentHashMap<>();
+ private static final long EMPTY_SIZE;
+ static
+ {
+ PrepareMessage msg = new PrepareMessage(nextTimeUUID(),
Collections.emptyList(), Collections.emptyList(), false, 0L, false,
PreviewKind.NONE);
+ ParticipateState participateState = new
ParticipateState(Clock.Global.clock(),
FBUtilities.getBroadcastAddressAndPort(), msg);
+ RepairJobDesc desc = new RepairJobDesc(null, null, null, null,
null);
+ EMPTY_SIZE = ObjectSizes.measure(new Job(Clock.Global.clock(),
participateState, desc));
Review Comment:
```suggestion
EMPTY_SIZE = ObjectSizes.measure(new Job(Clock.Global.clock(),
null, null));
```
could cleanup some code / imports...
##########
src/java/org/apache/cassandra/repair/state/SessionState.java:
##########
@@ -42,14 +54,18 @@ public enum State
public final String keyspace;
public final String[] cfnames;
public final CommonRange commonRange;
+ private final CoordinatorState coordinator;
private final ConcurrentMap<UUID, JobState> jobs = new
ConcurrentHashMap<>();
public final Phase phase = new Phase();
- public SessionState(Clock clock, TimeUUID parentRepairSession, String
keyspace, String[] cfnames, CommonRange commonRange)
+ private static final long EMPTY_SIZE = ObjectSizes.measure(new
SessionState(new CoordinatorState(SharedContext.Global.instance.clock(), 0, "",
new RepairOption(RepairParallelism.SEQUENTIAL, false, false, false, 8,
Collections.emptySet(), false, false, false, PreviewKind.NONE, false, false,
false, false)), SharedContext.Global.instance.clock(), "", new String[] {}, new
CommonRange(ImmutableSet.of(FBUtilities.getBroadcastAddressAndPort()),
Collections.emptySet(), Range.rangeSet(new Range<>(new
Murmur3Partitioner.LongToken(Long.MIN_VALUE), new
Murmur3Partitioner.LongToken(Long.MIN_VALUE))))));
Review Comment:
since you do `ObjectSize.measure` it would be good to do `null` than define
so much that isn't actually needed...
##########
src/java/org/apache/cassandra/repair/state/ParticipateState.java:
##########
@@ -148,16 +200,43 @@ public void accept()
}
}
- public static class Job extends AbstractState<Job.State, RepairJobDesc>
+ public static class Job extends AbstractState<Job.State, RepairJobDesc>
implements WeightedHierarchy.InternalNode
{
public enum State { ACCEPT, SNAPSHOT, VALIDATION, SYNC }
+ private final ParticipateState participateState;
private final AtomicReference<ValidationState> validation = new
AtomicReference<>(null);
private final ConcurrentMap<SyncState.Id, SyncState> syncs = new
ConcurrentHashMap<>();
+ private static final long EMPTY_SIZE;
+ static
+ {
+ PrepareMessage msg = new PrepareMessage(nextTimeUUID(),
Collections.emptyList(), Collections.emptyList(), false, 0L, false,
PreviewKind.NONE);
+ ParticipateState participateState = new
ParticipateState(Clock.Global.clock(),
FBUtilities.getBroadcastAddressAndPort(), msg);
+ RepairJobDesc desc = new RepairJobDesc(null, null, null, null,
null);
+ EMPTY_SIZE = ObjectSizes.measure(new Job(Clock.Global.clock(),
participateState, desc));
+ }
- public Job(Clock clock, RepairJobDesc desc)
+ public Job(Clock clock, ParticipateState participateState,
RepairJobDesc desc)
{
super(clock, desc, State.class);
+ this.participateState = participateState;
+ }
+
+ @Override
+ public WeightedHierarchy.Root root()
+ {
+ return participateState;
+ }
+
+ @Override
+ public long independentRetainedSize()
+ {
+ long size = EMPTY_SIZE;
+
+ // Excludes participateState since that's already measured as the
parent, and both validation and syncs propagate
+ // their updates back to the root so do not need to be counted
here.
+
+ return size;
Review Comment:
```suggestion
// Excludes participateState since that's already measured as
the parent, and both validation and syncs propagate
// their updates back to the root so do not need to be counted
here.
return EMPTY_SIZE;
```
##########
src/java/org/apache/cassandra/repair/state/ParticipateState.java:
##########
@@ -81,7 +129,11 @@ public Job job(RepairJobDesc desc)
public Job getOrCreateJob(RepairJobDesc desc)
{
- return jobs.computeIfAbsent(desc, d -> new Job(clock, d));
+ boolean isNew = !jobs.containsKey(desc);
+ Job job = jobs.computeIfAbsent(desc, d -> new Job(clock, this, d));
Review Comment:
this isn't thread safe...
--
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]
