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 15:03:51 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10013: MINOR: Remove ZK dependency for __transaction_state partition count
rondagostino opened a new pull request #10013:
URL: https://github.com/apache/kafka/pull/10013
`TransactionStateManager` currently uses ZooKeeper to obtain any non-default value for the `__transaction_state` partition count. ZooKeeper will not be available when the broker uses a Raft-based metadata quorum. This PR removes the hard dependency on ZooKeeper by allowing a function returning an `Int` to be provided instead. Providing a ZooKeeper client is still supported in `apply()` methods, but they transparently wrap the client with a function and pass that function instead.
Existing tests are sufficient to detect bugs and regressions.
### 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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r567952139
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
Can we not simply have a method in the companion object that we can lift into a function?
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r567966872
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
Yeah, I am suggesting you can still have it shared, but as a method in the companion object of one of the classes. And then you can have a simpler method name like `transactionStatePartitionCount` or something.
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r568046183
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
Closing this PR -- I consolidated these changes into https://github.com/apache/kafka/pull/10008 so we can review them together.
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r567962340
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
The logic to create a function `() => zkClient.getTopicPartitionCount(Topic.TRANSACTION_STATE_TOPIC_NAME).getOrElse(defaultValue)` is needed in 2 separate places (once in `TransactionCoordinator` and once in `TransactionStateManager`), so based on that I refactored it out into a common place here. I can remove this class if you prefer.
----------------------------------------------------------------
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 a change in pull request #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r568011675
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
I left a comment in the other PR. We do not want to get rid of startup, or have a callback function. See #10008. I agree with @ijuma that we should consolidate these into one PR to keep the discussion in one place.
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#discussion_r567967281
##########
File path: core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
##########
@@ -30,6 +30,13 @@ import org.apache.kafka.common.record.RecordBatch
import org.apache.kafka.common.requests.TransactionResult
import org.apache.kafka.common.utils.{LogContext, ProducerIdAndEpoch, Time}
+class TransactionStateTopicPartitionCountViaZooKeeper(zkClient: KafkaZkClient,
Review comment:
To clarify, it's a bit of an anti pattern to have a whole class when a method would do in Scala.
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#issuecomment-770924558
The combination of this PR and https://github.com/apache/kafka/pull/10009/ will remove hard ZooKeeper dependencies from the `TransactionCoordinator` and `TransactionStateManager` classes.
----------------------------------------------------------------
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 closed pull request #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
rondagostino closed pull request #10013:
URL: https://github.com/apache/kafka/pull/10013
----------------------------------------------------------------
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 #10013: MINOR: Remove ZK dependency for __transaction_state partition count
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10013:
URL: https://github.com/apache/kafka/pull/10013#issuecomment-771063821
Closing this PR -- I consolidated these changes into https://github.com/apache/kafka/pull/10008 so we can review them together.
----------------------------------------------------------------
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