dcapwell commented on code in PR #38:
URL: https://github.com/apache/cassandra-accord/pull/38#discussion_r1178090420
##########
accord-core/src/main/java/accord/local/CommandStores.java:
##########
@@ -65,19 +67,23 @@ CommandStores<?> create(NodeTimeService time,
private final DataStore store;
private final ProgressLog.Factory progressLogFactory;
private final CommandStore.Factory shardFactory;
+ private final RandomSource random;
- Supplier(NodeTimeService time, Agent agent, DataStore store,
ProgressLog.Factory progressLogFactory, CommandStore.Factory shardFactory)
+ Supplier(NodeTimeService time, Agent agent, DataStore store,
RandomSource random, ProgressLog.Factory progressLogFactory,
CommandStore.Factory shardFactory)
{
this.time = time;
this.agent = agent;
this.store = store;
+ this.random = random;
this.progressLogFactory = progressLogFactory;
this.shardFactory = shardFactory;
}
CommandStore create(int id, RangesForEpochHolder rangesForEpoch)
{
- return shardFactory.create(id, time, agent, store,
progressLogFactory, rangesForEpoch);
+ CommandStore store = shardFactory.create(id, time, agent,
this.store, progressLogFactory, rangesForEpoch);
+ store.execute(() -> CommandStore.register(store));
Review Comment:
> we already "unsafe" publish the CommandStore into the thread to setup the
thread field
Are you talking about `SingleThread`'s
```
executor.execute(() -> thread = Thread.currentThread());
```
This does not expose `this` for other callers to see, and uses a param
rather than a field, so we don't "unsafe publish"
> this is no different, and it would be cleaner.
I disagree, this is different as we are exposing the field for others to
consume. My understanding of JMM is that this publish isn't guarantied to be
visible in the new thread even after the constructor completes, which could
lead to a task seeing a partial `CommandStore`.
How about this compromise... we take a `CommandStore.Factory` and not
actually directly call `new`... so the `Thread` based `CommandStores` could do
`new` + `register` in that factory so this doesn't leak into `CommandStores`?
--
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]
