You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/01 01:56:53 UTC

[GitHub] [kafka] rondagostino opened a new pull request #10009: MINOR: Introduce ProducerIdGenerator trait

rondagostino opened a new pull request #10009:
URL: https://github.com/apache/kafka/pull/10009


   `ProducerIdManager` is an existing class that talks to ZooKeeper directly.  We won't have ZooKeeper when using a Raft-based metadata quorum, so we need an abstraction for the functionality of generating producer IDs.  This PR introduces `ProducerIdGenerator` for this purpose, and we pass an implementation when instantiating `TransactionCoordinator` rather than letting `TransactionCoordinator.apply()` itself always create a ZooKeeper-based instance.
   
   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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.

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

[GitHub] [kafka] ijuma commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r569022968



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -83,7 +83,7 @@ object TransactionCoordinator {
 class TransactionCoordinator(brokerId: Int,
                              txnConfig: TransactionConfig,
                              scheduler: Scheduler,
-                             producerIdManager: ProducerIdManager,
+                             producerIdGeneratorFactory: () => ProducerIdGenerator,

Review comment:
       Nit: since this is a function, I'd call it `createProduceIdGenerator` or a verb like that.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#issuecomment-772625293


   Two unrelated flaky failures:
   
   > Build / JDK 15 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testMultipleWorkersRejoining
   > Build / JDK 8 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSinkTaskStart


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma merged pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10009:
URL: https://github.com/apache/kafka/pull/10009


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] rondagostino commented on pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#issuecomment-770914261


   Only test failure is a known flaky test: https://issues.apache.org/jira/browse/KAFKA-7940


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r568649417



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       Yeah, I agree that the shutdown of producer id generator should move to `KafkaServer` if it's where we create it. Alternatively, `TransactionCoordinator` could take a factory (i.e. () => ProducerIdGenerator) for creating a `ProducerIdGenerator` and keep lifecycle management in `TransactionCoordinator`.

##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       I think the factory with the `apply` method would work reasonably well.

##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -83,7 +83,7 @@ object TransactionCoordinator {
 class TransactionCoordinator(brokerId: Int,
                              txnConfig: TransactionConfig,
                              scheduler: Scheduler,
-                             producerIdManager: ProducerIdManager,
+                             producerIdGeneratorFactory: () => ProducerIdGenerator,

Review comment:
       Nit: since this is a function, I'd call it `createProduceIdGenerator` or a verb like that.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r568649417



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       Yeah, I agree that the shutdown of producer id generator should move to `KafkaServer` if it's where we create it. Alternatively, `TransactionCoordinator` could take a factory (i.e. () => ProducerIdGenerator) for creating a `ProducerIdGenerator` and keep lifecycle management in `TransactionCoordinator`.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] cmccabe commented on pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#issuecomment-771029381


   This one looks good to me...


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r568650224



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       I think the factory with the `apply` method would work reasonably well.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#issuecomment-772519286


   Weird, red build again. Restarted it once again.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#issuecomment-772360801


   JDK 8 build exited (JDK 11 and 15 passed), so restarted the PR builder.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] mumrah commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r568133942



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       Since we're externalizing this trait (`ProducerIdGenerator`), it seems odd that TransactionCoordinator takes it as a dependency, but then later assumes the responsibility of closing it. I think we might need to move the shutdown call to the same place we create the instance, just so the ownership is clear.
   
   Alternatively (and maybe preferably?) we can keep the ProducerIdGenerator construction in the companion object and add a second `apply` method when we add the KIP-500 implementation.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] rondagostino commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r569014431



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       It felt weird to create the instance in `apply()` and hand it off to `TransactionCoordinator`, so I passed the factory and had the coordinator generate the instance.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] rondagostino commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r569014431



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       It felt weird to create the instance in `apply()` and hand it off to `TransactionCoordinator`, so I passed the factory and had the coordinator generate the instance.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] mumrah commented on a change in pull request #10009: MINOR: Introduce ProducerIdGenerator trait

Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #10009:
URL: https://github.com/apache/kafka/pull/10009#discussion_r568134453



##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -615,7 +615,7 @@ class TransactionCoordinator(brokerId: Int,
     info("Shutting down.")
     isActive.set(false)
     scheduler.shutdown()
-    producerIdManager.shutdown()
+    producerIdGenerator.shutdown()

Review comment:
       Either way, the new trait looks good 




----------------------------------------------------------------
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:
users@infra.apache.org