apurtell commented on code in PR #2489:
URL: https://github.com/apache/phoenix/pull/2489#discussion_r3455907791
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogReplayService.java:
##########
@@ -77,14 +91,33 @@ public class ReplicationLogReplayService {
*/
public static final int
DEFAULT_REPLICATION_REPLAY_SERVICE_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 30;
+ private static final long CONSISTENCY_POINT_CACHE_TTL_SECONDS = 30;
+
private static volatile ReplicationLogReplayService instance;
private final Configuration conf;
Review Comment:
The two test-only constructors in `ReplicationLogReplayService` do not
initialize the blank-final field `conf`. This will break the build.
```
ReplicationLogReplayService.java:116: error: variable conf might not have
been initialized
ReplicationLogReplayService.java:121: error: variable conf might not have
been initialized
```
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogReplayService.java:
##########
@@ -77,14 +91,33 @@ public class ReplicationLogReplayService {
*/
public static final int
DEFAULT_REPLICATION_REPLAY_SERVICE_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 30;
+ private static final long CONSISTENCY_POINT_CACHE_TTL_SECONDS = 30;
+
private static volatile ReplicationLogReplayService instance;
private final Configuration conf;
private ScheduledExecutorService scheduler;
private volatile boolean isRunning = false;
+ private final Supplier<Long> cachedConsistencyPoint;
private ReplicationLogReplayService(final Configuration conf) {
this.conf = conf;
+ this.cachedConsistencyPoint = Suppliers.memoizeWithExpiration(() -> {
Review Comment:
`Suppliers.memoizeWithExpiration` uses `synchronized(this)` to serialize
loads, but that lock is on the supplier object. This establishes no
happens-before relationship with the replay thread's writes to
`lastRoundInSync`.
So you may end up advancing the consistency point too far back, or even
getting a a `null` back from the supplier even after the replay thread has
already set it, spuriously throwing an `IOException`. The net effect is
sometimes a major compaction won't drop delete markers when it should.
*Something* needs to enforce the happens-before relationship. Perhaps
`volatile ReplicationRound lastRoundInSync` , or consider
`AtomicReference<ReplicationRound>`. Or redo this.
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogReplayService.java:
##########
@@ -50,6 +51,17 @@ public class ReplicationLogReplayService {
*/
public static final boolean DEFAULT_REPLICATION_REPLAY_ENABLED = false;
+ /**
+ * Configuration key for enabling/disabling the replication compaction guard
+ */
+ public static final String REPLICATION_COMPACTION_GUARD_ENABLED =
+ "phoenix.replication.compaction.guard.enabled";
Review Comment:
This is unnecessary configuration and an accident waiting to happen.
--
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]
