chia7712 commented on code in PR #15670:
URL: https://github.com/apache/kafka/pull/15670#discussion_r1556404908


##########
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##########
@@ -16,11 +16,43 @@
  */
 package org.apache.kafka.coordinator.transaction;
 
+import org.apache.kafka.common.config.TopicConfig;
+import org.apache.kafka.server.config.ServerTopicConfigSynonyms;
+import org.apache.kafka.storage.internals.log.ProducerStateManagerConfig;
+
 public class TransactionLogConfig {

Review Comment:
   In order to make it be a pure constants class, could you please add 
following changes?
   
   1. rename it to `TransactionLogConfigs`?
   2. add `final`
   3. add private constructor



##########
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##########
@@ -16,11 +16,43 @@
  */
 package org.apache.kafka.coordinator.transaction;
 
+import org.apache.kafka.common.config.TopicConfig;
+import org.apache.kafka.server.config.ServerTopicConfigSynonyms;
+import org.apache.kafka.storage.internals.log.ProducerStateManagerConfig;
+
 public class TransactionLogConfig {
     // Log-level config default values
-    public static final int DEFAULT_NUM_PARTITIONS = 50;
-    public static final int DEFAULT_SEGMENT_BYTES = 100 * 1024 * 1024;
-    public static final short DEFAULT_REPLICATION_FACTOR = 3;
-    public static final int DEFAULT_MIN_IN_SYNC_REPLICAS = 2;
-    public static final int DEFAULT_LOAD_BUFFER_SIZE = 5 * 1024 * 1024;
+    public static final String TRANSACTIONS_TOPIC_PARTITIONS_CONFIG = 
"transaction.state.log.num.partitions";
+    public static final int TRANSACTIONS_TOPIC_PARTITIONS_DEFAULT = 50;
+    public static final String TRANSACTIONS_TOPIC_PARTITIONS_DOC = "The number 
of partitions for the transaction topic (should not change after deployment).";
+
+    public static final String TRANSACTIONS_TOPIC_SEGMENT_BYTES_CONFIG = 
"transaction.state.log.segment.bytes";
+    public static final int TRANSACTIONS_TOPIC_SEGMENT_BYTES_DEFAULT = 100 * 
1024 * 1024;
+    public static final String TRANSACTIONS_TOPIC_SEGMENT_BYTES_DOC = "The 
transaction topic segment bytes should be kept relatively small in order to 
facilitate faster log compaction and cache loads";
+
+    public static final String TRANSACTIONS_TOPIC_REPLICATION_FACTOR_CONFIG = 
"transaction.state.log.replication.factor";
+    public static final short TRANSACTIONS_TOPIC_REPLICATION_FACTOR_DEFAULT = 
3;
+    public static final String TRANSACTIONS_TOPIC_REPLICATION_FACTOR_DOC = 
"The replication factor for the transaction topic (set higher to ensure 
availability). " +
+            "Internal topic creation will fail until the cluster size meets 
this replication factor requirement.";
+
+    public static final String TRANSACTIONS_TOPIC_MIN_ISR_CONFIG = 
"transaction.state.log.min.isr";
+    public static final int TRANSACTIONS_TOPIC_MIN_ISR_DEFAULT = 2;
+    public static final String TRANSACTIONS_TOPIC_MIN_ISR_DOC = "Overridden " 
+ 
ServerTopicConfigSynonyms.serverSynonym(TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG)
 + " config for the transaction topic.";

Review Comment:
   Maybe we can revise the docs to avoid depending on those modules directly?



##########
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##########
@@ -16,11 +16,43 @@
  */
 package org.apache.kafka.coordinator.transaction;
 
+import org.apache.kafka.common.config.TopicConfig;
+import org.apache.kafka.server.config.ServerTopicConfigSynonyms;
+import org.apache.kafka.storage.internals.log.ProducerStateManagerConfig;
+
 public class TransactionLogConfig {
     // Log-level config default values
-    public static final int DEFAULT_NUM_PARTITIONS = 50;
-    public static final int DEFAULT_SEGMENT_BYTES = 100 * 1024 * 1024;
-    public static final short DEFAULT_REPLICATION_FACTOR = 3;
-    public static final int DEFAULT_MIN_IN_SYNC_REPLICAS = 2;
-    public static final int DEFAULT_LOAD_BUFFER_SIZE = 5 * 1024 * 1024;
+    public static final String TRANSACTIONS_TOPIC_PARTITIONS_CONFIG = 
"transaction.state.log.num.partitions";
+    public static final int TRANSACTIONS_TOPIC_PARTITIONS_DEFAULT = 50;
+    public static final String TRANSACTIONS_TOPIC_PARTITIONS_DOC = "The number 
of partitions for the transaction topic (should not change after deployment).";
+
+    public static final String TRANSACTIONS_TOPIC_SEGMENT_BYTES_CONFIG = 
"transaction.state.log.segment.bytes";
+    public static final int TRANSACTIONS_TOPIC_SEGMENT_BYTES_DEFAULT = 100 * 
1024 * 1024;
+    public static final String TRANSACTIONS_TOPIC_SEGMENT_BYTES_DOC = "The 
transaction topic segment bytes should be kept relatively small in order to 
facilitate faster log compaction and cache loads";
+
+    public static final String TRANSACTIONS_TOPIC_REPLICATION_FACTOR_CONFIG = 
"transaction.state.log.replication.factor";
+    public static final short TRANSACTIONS_TOPIC_REPLICATION_FACTOR_DEFAULT = 
3;
+    public static final String TRANSACTIONS_TOPIC_REPLICATION_FACTOR_DOC = 
"The replication factor for the transaction topic (set higher to ensure 
availability). " +
+            "Internal topic creation will fail until the cluster size meets 
this replication factor requirement.";
+
+    public static final String TRANSACTIONS_TOPIC_MIN_ISR_CONFIG = 
"transaction.state.log.min.isr";
+    public static final int TRANSACTIONS_TOPIC_MIN_ISR_DEFAULT = 2;
+    public static final String TRANSACTIONS_TOPIC_MIN_ISR_DOC = "Overridden " 
+ 
ServerTopicConfigSynonyms.serverSynonym(TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG)
 + " config for the transaction topic.";
+
+    public static final String TRANSACTIONS_LOAD_BUFFER_SIZE_CONFIG = 
"transaction.state.log.load.buffer.size";
+    public static final int TRANSACTIONS_LOAD_BUFFER_SIZE_DEFAULT = 5 * 1024 * 
1024;
+    public static final String TRANSACTIONS_LOAD_BUFFER_SIZE_DOC = "Batch size 
for reading from the transaction log segments when loading producer ids and 
transactions into the cache (soft-limit, overridden if records are too large).";
+
+    public static final String 
TRANSACTION_PARTITION_VERIFICATION_ENABLE_CONFIG = 
ProducerStateManagerConfig.TRANSACTION_VERIFICATION_ENABLED;
+    public static final boolean 
TRANSACTION_PARTITION_VERIFICATION_ENABLE_DEFAULT = true;
+    public static final String TRANSACTION_PARTITION_VERIFICATION_ENABLE_DOC = 
"Enable verification that checks that the partition has been added to the 
transaction before writing transactional records to the partition";
+
+    public static final String PRODUCER_ID_EXPIRATION_MS_CONFIG = 
ProducerStateManagerConfig.PRODUCER_ID_EXPIRATION_MS;

Review Comment:
   not sure whether there are other migration in the future. It seems to me 
`PRODUCER_ID_EXPIRATION_MS` can be moved to `TransactionLogConfig` as the 
module which is using `ProducerStateManagerConfig.PRODUCER_ID_EXPIRATION_MS` 
can access `TransactionLogConfig`. By doing that, we can de-couple of 
`transaction-coordinator` and `storage`



-- 
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: jira-unsubscr...@kafka.apache.org

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

Reply via email to