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/03/14 18:16:04 UTC
[GitHub] [kafka] C0urante opened a new pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
C0urante opened a new pull request #10315:
URL: https://github.com/apache/kafka/pull/10315
[Jira](https://issues.apache.org/jira/browse/KAFKA-12463)
Changes sink tasks to use the `CooperativeStickyAssignor` by default, in a fashion that permits rolling upgrades of Connect workers and retains existing support for worker-level and connector-level overrides for the consumer assignor property.
### 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] C0urante commented on pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#issuecomment-816388437
Closing as this will likely be accomplished by KIP-726; can reopen if necessary.
--
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] C0urante commented on a change in pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#discussion_r597034374
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -698,6 +700,8 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState,
consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
consumerProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
consumerProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
+ consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG,
+ CooperativeStickyAssignor.class.getName() + "," + RangeAssignor.class.getName());
Review comment:
Yeah, a comment is definitely warranted, good call. 👌
We might use the `RoundRobinAssignor`, but I don't think we have to settle for that just yet. There's a couple of bugs/improvements in the way, but at least one of them is being actively worked on (https://github.com/apache/kafka/pull/10345) and I'm happy to take care of the other (https://issues.apache.org/jira/browse/KAFKA-12487).
I converted this to a draft since it's definitely not ready for merge yet and it's unclear which path forward we might want to take. We might want to reason through this carefully since we only get one shot to do this kind of automated upgrade before the next one gets more complicated (the list of assignors will grow from one to two this time around; it'll either have to grow from two to three the next, or we'll have to risk breaking changes for users who skip an upgrade step).
I'll try to do an analysis of the pros/cons of the `CooperativeStickyAssignor` vs the `RoundRobinAssignor` with regards to sink connectors on the Jira ticket; I'm actually leaning a tiny bit toward the `RoundRobinAssignor` but I might be suffering from tunnel vision due to recent misadventures with other assignors.
--
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] rhauch commented on a change in pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
rhauch commented on a change in pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#discussion_r597007027
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -698,6 +700,8 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState,
consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
consumerProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
consumerProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
+ consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG,
+ CooperativeStickyAssignor.class.getName() + "," + RangeAssignor.class.getName());
Review comment:
As discussed on the [Jira issue](https://issues.apache.org/jira/browse/KAFKA-12463), I think you're suggesting that we use `RoundRobinAssignor` instead of `CooperativeStickyAssignor` due to the issue you found with Connect using the latter ([KAFKA-12487](https://issues.apache.org/jira/browse/KAFKA-12487)).
Also, it's probably worthwhile to add a comment here about why we're using _two_ assignors rather than _only_ the cooperative (or round robin) assignor. Maybe something like:
```
// Prefer the cooperative assignor, but allow old range assignor during rolling upgrades
// from Connect versions that just used the range assignor (the default)
```
--
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] C0urante closed pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante closed pull request #10315:
URL: https://github.com/apache/kafka/pull/10315
--
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] C0urante commented on a change in pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#discussion_r597034374
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -698,6 +700,8 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState,
consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
consumerProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
consumerProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
+ consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG,
+ CooperativeStickyAssignor.class.getName() + "," + RangeAssignor.class.getName());
Review comment:
Yeah, a comment is definitely warranted, good call. 👌
We might use the `RoundRobinAssignor`, but I don't think we have to settle for that just yet. There's a couple of bugs/improvements in the way, but at least one of them is being actively worked on (https://github.com/apache/kafka/pull/10345) and I'm happy to take care of the other ([KAFKA-12487](https://issues.apache.org/jira/browse/KAFKA-12487)).
I converted this to a draft since it's definitely not ready for merge yet and it's unclear which path forward we might want to take. We might want to reason through this carefully since we only get one shot to do this kind of automated upgrade before the next one gets more complicated (the list of assignors will grow from one to two this time around; it'll either have to grow from two to three the next, or we'll have to risk breaking changes for users who skip an upgrade step).
I'll try to do an analysis of the pros/cons of the `CooperativeStickyAssignor` vs the `RoundRobinAssignor` with regards to sink connectors on the Jira ticket; I'm actually leaning a tiny bit toward the `RoundRobinAssignor` but I might be suffering from tunnel vision due to recent misadventures with other assignors.
--
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] C0urante commented on a change in pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#discussion_r597034374
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -698,6 +700,8 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState,
consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
consumerProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
consumerProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
+ consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG,
+ CooperativeStickyAssignor.class.getName() + "," + RangeAssignor.class.getName());
Review comment:
Yeah, a comment is definitely warranted, good call.
We might use the `RoundRobinAssignor` but don't think we have to settle for that just yet. There's a couple of bugs/improvements in the way, but at least one of them is being actively worked on (https://github.com/apache/kafka/pull/10345) and I'm happy to take care of the other.
I converted this to a draft since it's definitely not ready for merge yet and it's unclear which path forward we might want to take. We might want to reason through this carefully since we only get one shot to do this kind of automated upgrade before the next one gets more complicated (the list of assignors will grow from one to two this time around; it'll either have to grow from two to three the next, or we'll have to risk breaking changes for users who skip an upgrade step).
I'll try to do an analysis of the pros/cons of the `CooperativeStickyAssignor` vs the `RoundRobinAssignor` with regards to sink connectors on the Jira ticket; I'm actually leaning a tiny bit toward the `RoundRobinAssignor` but I might be suffering from tunnel vision due to recent misadventures with other assignors.
--
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] C0urante commented on a change in pull request #10315: KAFKA-12463: Update default sink task partition assignor to cooperative sticky assignor
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #10315:
URL: https://github.com/apache/kafka/pull/10315#discussion_r597034374
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -698,6 +700,8 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState,
consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
consumerProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
consumerProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArrayDeserializer");
+ consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG,
+ CooperativeStickyAssignor.class.getName() + "," + RangeAssignor.class.getName());
Review comment:
Yeah, a comment is definitely warranted, good call. 👌
We might use the `RoundRobinAssignor` but don't think we have to settle for that just yet. There's a couple of bugs/improvements in the way, but at least one of them is being actively worked on (https://github.com/apache/kafka/pull/10345) and I'm happy to take care of the other.
I converted this to a draft since it's definitely not ready for merge yet and it's unclear which path forward we might want to take. We might want to reason through this carefully since we only get one shot to do this kind of automated upgrade before the next one gets more complicated (the list of assignors will grow from one to two this time around; it'll either have to grow from two to three the next, or we'll have to risk breaking changes for users who skip an upgrade step).
I'll try to do an analysis of the pros/cons of the `CooperativeStickyAssignor` vs the `RoundRobinAssignor` with regards to sink connectors on the Jira ticket; I'm actually leaning a tiny bit toward the `RoundRobinAssignor` but I might be suffering from tunnel vision due to recent misadventures with other assignors.
--
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