apurtell commented on code in PR #2489:
URL: https://github.com/apache/phoenix/pull/2489#discussion_r3463746339
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogReplayService.java:
##########
@@ -77,14 +81,35 @@ 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(() -> {
+ try {
+ return getConsistencyPoint();
+ } catch (IOException | SQLException e) {
+ throw new RuntimeException("Failed to fetch consistency point", e);
+ }
+ }, CONSISTENCY_POINT_CACHE_TTL_SECONDS, TimeUnit.SECONDS);
+ }
+
+ private ReplicationLogReplayService(long fixedConsistencyPoint) {
+ this.conf = null;
+ this.cachedConsistencyPoint = () -> fixedConsistencyPoint;
+ }
+
+ private ReplicationLogReplayService(Supplier<Long> supplier) {
+ this.conf = null;
Review Comment:
Isn't this bad? Various places will NPE because of attempt to deference
`null` to get a conf value. If we need a conf to vary our behavior, it should
be passed in by a higher layer. If we need a conf because some API we call
needs it, we should create it with `new Configuration()`.
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogReplayService.java:
##########
@@ -77,14 +81,35 @@ 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(() -> {
+ try {
+ return getConsistencyPoint();
+ } catch (IOException | SQLException e) {
+ throw new RuntimeException("Failed to fetch consistency point", e);
+ }
+ }, CONSISTENCY_POINT_CACHE_TTL_SECONDS, TimeUnit.SECONDS);
+ }
+
+ private ReplicationLogReplayService(long fixedConsistencyPoint) {
+ this.conf = null;
Review Comment:
Isn't this bad? Various places will NPE because of attempt to deference
`null` to get a conf value. If we need a conf to vary our behavior, it should
be passed in by a higher layer. If we need a conf because some API we call
needs it, we should create it with `new Configuration()`.
--
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]
