[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #39: CASSANDRA-18221: Add AccordConfig to track accord specific configs

["dcapwell (via GitHub)" <gi...@apache.org>]

dcapwell commented on code in PR #39:
URL: https://github.com/apache/cassandra-accord/pull/39#discussion_r1157505204


##########
accord-core/src/main/java/accord/impl/SimpleProgressLog.java:
##########
@@ -88,11 +89,13 @@ boolean isAtLeastCommitted()
     enum DisseminateStatus { NotExecuted, Durable, Done }
 
     final Node node;
+    final AccordConfig config;
     final List<Instance> instances = new CopyOnWriteArrayList<>();
 
-    public SimpleProgressLog(Node node)
+    public SimpleProgressLog(Node node, AccordConfig config)

Review Comment:
   *should* `Node` expose config?  That way you don't need to pass all over?  



##########
accord-core/src/main/java/accord/impl/SimpleProgressLog.java:
##########
@@ -29,6 +29,7 @@
 
 import javax.annotation.Nullable;
 
+import accord.utils.AccordConfig;

Review Comment:
   I feel we should have `accord.config` package, this is likely to expand over time



##########
accord-core/src/main/java/accord/impl/SimpleProgressLog.java:
##########
@@ -822,7 +825,7 @@ void ensureScheduled()
                 return;
 
             isScheduled = true;
-            node.scheduler().once(() -> commandStore.execute(PreLoadContext.empty(), ignore -> run()).begin(commandStore.agent()), 200L, TimeUnit.MILLISECONDS);
+            node.scheduler().once(() -> commandStore.execute(PreLoadContext.empty(), ignore -> run()).begin(commandStore.agent()), config.progress_log_scheduler_delay_in_ms, TimeUnit.MILLISECONDS);

Review Comment:
   we are trying to move away from `_in_ms` postfix... this may call out that we should likely move some of the config classes to accord?



##########
accord-core/src/main/java/accord/utils/AccordConfig.java:
##########
@@ -0,0 +1,11 @@
+package accord.utils;
+
+public class AccordConfig {
+    public long progress_log_scheduler_delay_in_ms = 200L;
+
+    public AccordConfig(long progress_log_scheduler_delay_in_ms) {
+        this.progress_log_scheduler_delay_in_ms = progress_log_scheduler_delay_in_ms;
+    }

Review Comment:
   this constructor isn't something we can maintain, as we add more and more configs this will become an issue



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org