You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mjsax (via GitHub)" <gi...@apache.org> on 2023/04/07 19:52:58 UTC
[GitHub] [kafka] mjsax opened a new pull request, #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
mjsax opened a new pull request, #13527:
URL: https://github.com/apache/kafka/pull/13527
This reverts commit d9b139220ee253da673af44d58dc87bd184188f1.
KIP-878 implementation did not make any progress, so we need to revert the public API changes which are not functional right now.
--
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] mjsax commented on a diff in pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #13527:
URL: https://github.com/apache/kafka/pull/13527#discussion_r1167196102
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/assignment/HighAvailabilityTaskAssignorTest.java:
##########
@@ -71,7 +67,23 @@
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.fail;
-public class HighAvailabilityTaskAssignorTest {
+public class HighAvailabilityTaskAssignorTest {
+ private final AssignmentConfigs configWithoutStandbys = new AssignmentConfigs(
+ /*acceptableRecoveryLag*/ 100L,
+ /*maxWarmupReplicas*/ 2,
+ /*numStandbyReplicas*/ 0,
+ /*probingRebalanceIntervalMs*/ 60 * 1000L,
+ /*rackAwareAssignmentTags*/ EMPTY_RACK_AWARE_ASSIGNMENT_TAGS
+ );
+
+ private final AssignmentConfigs configWithStandbys = new AssignmentConfigs(
Review Comment:
Sure. Actually don't know why the PR removed these -- don't have enough context on the details -- just did a `git revert`.
You wanna do a PR?
--
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] bbejeck commented on a diff in pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "bbejeck (via GitHub)" <gi...@apache.org>.
bbejeck commented on code in PR #13527:
URL: https://github.com/apache/kafka/pull/13527#discussion_r1167075556
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/AssignorConfiguration.java:
##########
@@ -261,41 +261,29 @@ public interface AssignmentListener {
void onAssignmentComplete(final boolean stable);
}
- /**
- * NOTE: any StreamsConfig you add here MUST be passed in to the consumer via
Review Comment:
This doesn't seem specific to KIP-878 - should it remain?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/assignment/HighAvailabilityTaskAssignorTest.java:
##########
@@ -71,7 +67,23 @@
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.fail;
-public class HighAvailabilityTaskAssignorTest {
+public class HighAvailabilityTaskAssignorTest {
+ private final AssignmentConfigs configWithoutStandbys = new AssignmentConfigs(
+ /*acceptableRecoveryLag*/ 100L,
+ /*maxWarmupReplicas*/ 2,
+ /*numStandbyReplicas*/ 0,
+ /*probingRebalanceIntervalMs*/ 60 * 1000L,
+ /*rackAwareAssignmentTags*/ EMPTY_RACK_AWARE_ASSIGNMENT_TAGS
+ );
+
+ private final AssignmentConfigs configWithStandbys = new AssignmentConfigs(
Review Comment:
As a side note, I think these two inner classes would benefit from using the `Builder` pattern. While the comments are useful here, it's hard to discern what the different parameters are when looking at the individual tests. I'm not suggesting this be done in this PR but in a follow-up one if you agree.
--
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] mjsax commented on a diff in pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #13527:
URL: https://github.com/apache/kafka/pull/13527#discussion_r1167194742
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/AssignorConfiguration.java:
##########
@@ -261,41 +261,29 @@ public interface AssignmentListener {
void onAssignmentComplete(final boolean stable);
}
- /**
- * NOTE: any StreamsConfig you add here MUST be passed in to the consumer via
Review Comment:
Maybe -- I literally just just did a `git revert` -- but easy to keep.
--
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] mjsax commented on pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on PR #13527:
URL: https://github.com/apache/kafka/pull/13527#issuecomment-1509166017
Merged to `trunk` and cherry-picked to `3.5` branch.
--
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] mjsax merged pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax merged PR #13527:
URL: https://github.com/apache/kafka/pull/13527
--
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] bbejeck commented on pull request #13527: Revert "KAFKA-14318: KIP-878, Introduce partition autoscaling configs (#12962)"
Posted by "bbejeck (via GitHub)" <gi...@apache.org>.
bbejeck commented on PR #13527:
URL: https://github.com/apache/kafka/pull/13527#issuecomment-1508982838
I looked at the build and the test failures are unrelated to this PR
--
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