sashapolo commented on code in PR #5134:
URL: https://github.com/apache/ignite-3/pull/5134#discussion_r1938938103
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -124,7 +124,7 @@ public class PartitionListener implements RaftGroupListener
{
private final UUID localNodeId;
- private Set<String> currentGroupTopology;
+ private volatile Set<String> currentGroupTopology;
Review Comment:
This is a little tricky. Previously, this variable was only written in
`onConfigurationCommitted` and read in `onWrite`, which means it was only
accessed by the single Raft write thread (`onConfigurationCommitted` is called
from this thread, right?). Now we have a new entry point:
`onConfigurationCommitted` can be called by the aggregate zone-level listener,
when we add a new table-level listener to it, which can happen in an arbitrary
thread. I think we can still avoid `volatile` here, because:
1. If `onConfigurationCommitted` was called from the Raft thread, then the
next read would be from the Raft thread, so no synchronization is needed;
2. If `onConfigurationCommitted` is called when we are adding a new
listener, then there will be a `put` in a `ConcurrentMap`, followed by a `get`
from the same map, when calling `onWrite`.
However, these guarantees seem to brittle to me and I'm not sure I'm not
missing something here (we are still reading through a race and may be
overlooking some side effects).
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
