xinyuiscool commented on a change in pull request #1190: SAMZA-2352: Use min.compaction.lag.ms to avoid compacting the Kafka changelog topic URL: https://github.com/apache/samza/pull/1190#discussion_r336123786
########## File path: samza-core/src/main/java/org/apache/samza/config/StorageConfig.java ########## @@ -52,6 +52,9 @@ public static final boolean DEFAULT_DISALLOW_LARGE_MESSAGES = false; public static final String DROP_LARGE_MESSAGES = STORE_PREFIX + "%s.drop.large.messages"; public static final boolean DEFAULT_DROP_LARGE_MESSAGES = false; + // The log compaction lag time for transactional state change log + public static final String CHANGELOG_MIN_COMPACTION_LAG_MS = STORE_PREFIX + "%s.changelog.kafka.min.compaction.lag.ms"; Review comment: I think we are in an inconsistent position for this config. On one hand, the logic in KafkaConfig.getChangelogKafkaProperties() already allows overriding any kafka properties by using stores.x.changelog.kafka..... So the user can already override things like replication.factor using stores.x.changelog.kafka.replication.factor. The changelog configs in StorageConfig make a second way to configure it, which is problematic since the user can use the original kafka override to do it and StorageConfig code getters won't return the correct value. Seems like an existing bug to me already. Not sure how many use cases are using those two configs, but it's problematic in the long run. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
