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