tkhurana commented on PR #2489:
URL: https://github.com/apache/phoenix/pull/2489#issuecomment-4771461654
When the cache is cold or expired, the delegate calls getConsistencyPoint() →
ReplicationLogDiscoveryReplay.getConsistencyPoint(), which reads
lastRoundInSync (ReplicationLogDiscoveryReplay.java:570-574, 584-585):
private ReplicationRound lastRoundInSync; // line 106 — plain field, NOT
volatile
That field is written by the background replay thread (setLastRoundInSync
at :328, and init at :242/:249) and has no volatile modifier and no
synchronization (:438-448).
Before this PR, getConsistencyPoint() was only ever called from the single
replay scheduler thread (REPLICATION_REPLAY_SERVICE_EXECUTOR_THREAD_COUNT = 1,
invoked at
:287/:349), so the field was effectively thread-confined.
This PR makes HBase compaction threads (the large/small compaction pools —
commonly >1, so multiple CompactionScanner constructors run in parallel on one
RS) call
getConsistencyPoint() for the first time from outside that thread. The
synchronized(this) inside the Guava supplier serializes the compaction threads
against each other,
but it locks the supplier object — it establishes no happens-before with
the replay thread's writes to lastRoundInSync. Consequences:
- Visibility: a compaction thread can observe a stale lastRoundInSync, or
even null after the replay thread has set it — which trips the throw new
IOException("...lastRoundInSync is not initialized") at :577/:588 →
resolveConsistencyPoint catches it → returns 0L → retain everything (the silent
fail-closed path from
the main review). So the race doesn't corrupt data, but it can spuriously
and silently disable purging.
- replicationReplayState is an AtomicReference (:109) so that read is
fine; lastRoundInSync is the unprotected one.
--
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]
