You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "jolshan (via GitHub)" <gi...@apache.org> on 2023/05/26 22:59:52 UTC

[GitHub] [kafka] jolshan opened a new pull request, #13770: MINOR: Add config to producerStateManager config

jolshan opened a new pull request, #13770:
URL: https://github.com/apache/kafka/pull/13770

   Originally part of https://github.com/apache/kafka/pull/13608/files. Since there are so many files changed, I decided to just pull this out into its own PR.
   
   I have moved this config into producer state manager so it can be checked easily under the log lock when we are about to append.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] dajac commented on a diff in pull request #13770: MINOR: Add config to producerStateManager config

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13770:
URL: https://github.com/apache/kafka/pull/13770#discussion_r1210559849


##########
storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManagerConfig.java:
##########
@@ -24,8 +24,11 @@ public class ProducerStateManagerConfig {
     public static final Set<String> RECONFIGURABLE_CONFIGS = Collections.singleton(PRODUCER_ID_EXPIRATION_MS);
     private volatile int producerIdExpirationMs;
 
-    public ProducerStateManagerConfig(int producerIdExpirationMs) {
+    private volatile boolean transactionVerificationEnabled;

Review Comment:
   In this case, the `volatile` is likely not required if we don't update it.



-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] jolshan commented on pull request #13770: MINOR: Add transaction verification config to producerStateManager config

Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #13770:
URL: https://github.com/apache/kafka/pull/13770#issuecomment-1569075902

   Checked the build -- streams upgrade tests had some issue that failed jdk 17 build.
   There were also two test failures -- 
   
   [Build / JDK 8 and Scala 2.12 / org.apache.kafka.tools.MetadataQuorumCommandErrorTest.testRelativeTimeMs()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13770/3/testReport/junit/org.apache.kafka.tools/MetadataQuorumCommandErrorTest/Build___JDK_8_and_Scala_2_12___testRelativeTimeMs__/)
   [Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.[1] true](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13770/3/testReport/junit/org.apache.kafka.streams.integration/SmokeTestDriverIntegrationTest/Build___JDK_11_and_Scala_2_13____1__true/)
   
   Given the nature of this change -- basically a no-op except for some test files that all passed. I will go ahead and merge.


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] jolshan merged pull request #13770: MINOR: Add transaction verification config to producerStateManager config

Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan merged PR #13770:
URL: https://github.com/apache/kafka/pull/13770


-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] jolshan commented on a diff in pull request #13770: MINOR: Add config to producerStateManager config

Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #13770:
URL: https://github.com/apache/kafka/pull/13770#discussion_r1210529153


##########
storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManagerConfig.java:
##########
@@ -24,8 +24,11 @@ public class ProducerStateManagerConfig {
     public static final Set<String> RECONFIGURABLE_CONFIGS = Collections.singleton(PRODUCER_ID_EXPIRATION_MS);
     private volatile int producerIdExpirationMs;
 
-    public ProducerStateManagerConfig(int producerIdExpirationMs) {
+    private volatile boolean transactionVerificationEnabled;

Review Comment:
   I considered this, but the issue is that we currently start up a thread in startup to handle verification. I'm not sure we can do this dynamically.



##########
storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManagerConfig.java:
##########
@@ -24,8 +24,11 @@ public class ProducerStateManagerConfig {
     public static final Set<String> RECONFIGURABLE_CONFIGS = Collections.singleton(PRODUCER_ID_EXPIRATION_MS);
     private volatile int producerIdExpirationMs;
 
-    public ProducerStateManagerConfig(int producerIdExpirationMs) {
+    private volatile boolean transactionVerificationEnabled;

Review Comment:
   can do



-- 
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-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] dajac commented on a diff in pull request #13770: MINOR: Add config to producerStateManager config

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13770:
URL: https://github.com/apache/kafka/pull/13770#discussion_r1209855532


##########
core/src/main/java/kafka/server/builders/LogManagerBuilder.java:
##########
@@ -111,7 +111,12 @@ public LogManagerBuilder setMaxTransactionTimeoutMs(int maxTransactionTimeoutMs)
     }
 
     public LogManagerBuilder setMaxProducerIdExpirationMs(int maxProducerIdExpirationMs) {
-        this.producerStateManagerConfig = new ProducerStateManagerConfig(maxProducerIdExpirationMs);
+        this.producerStateManagerConfig = new ProducerStateManagerConfig(maxProducerIdExpirationMs, false);

Review Comment:
   Should we remove `setMaxProducerIdExpirationMs` and use `setProducerStateManagerConfig` everywhere?



##########
storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManagerConfig.java:
##########
@@ -24,8 +24,11 @@ public class ProducerStateManagerConfig {
     public static final Set<String> RECONFIGURABLE_CONFIGS = Collections.singleton(PRODUCER_ID_EXPIRATION_MS);
     private volatile int producerIdExpirationMs;
 
-    public ProducerStateManagerConfig(int producerIdExpirationMs) {
+    private volatile boolean transactionVerificationEnabled;

Review Comment:
   small nit: I would put an empty line before `producerIdExpirationMs` and remove the one between `producerIdExpirationMs` and `transactionVerificationEnabled`.



##########
storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManagerConfig.java:
##########
@@ -24,8 +24,11 @@ public class ProducerStateManagerConfig {
     public static final Set<String> RECONFIGURABLE_CONFIGS = Collections.singleton(PRODUCER_ID_EXPIRATION_MS);
     private volatile int producerIdExpirationMs;
 
-    public ProducerStateManagerConfig(int producerIdExpirationMs) {
+    private volatile boolean transactionVerificationEnabled;

Review Comment:
   Do we plan to make it dynamic?



-- 
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-unsubscribe@kafka.apache.org

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