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/26 05:06:36 UTC
[GitHub] [kafka] ableegoldman opened a new pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
ableegoldman opened a new pull request #10411:
URL: https://github.com/apache/kafka/pull/10411
Removes deprecated flags from StreamsResetter:
"--zookeeper" flag (deprecated in 1.0 via KIP-198)
"--execute" flag (deprecated in 1.1 via KIP-171)
--
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] ableegoldman commented on pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#issuecomment-817046460
Unrelated test failure in `InternalTopicManagerTest.shouldOnlyRetryNotSuccessfulFuturesDuringSetup()` (filed https://issues.apache.org/jira/browse/KAFKA-12650)
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604532122
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
I tend to agree. -- My main point was that I wanted to raise awareness and that we should make a conscious decision about it.
Might also be good to hear from @guozhangwang about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#issuecomment-811364280
Unrelated test failure: `streams.integration.StoreQueryIntegrationTest.shouldQuerySpecificActivePartitionStores`
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r603719685
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +246,10 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
Review comment:
@mjsax PR is ready for review again, lmk what you think
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604547699
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
Some weird things I noticed: First, the ticket says `--execute` was deprecated in 1.1, but it doesn't even exist in 1.0 (or earlier AFAICT but I did not check all branches). Did we...add it and deprecate it in the same release? Huh?
Also, I just read KIP-171 and it's honestly super confusing. Regarding the deprecation, it says `"Current option '–execute' will be marked as "going-to-be-deprecated" and replaced by 'dry-run' parameter, until it gets replaced."`
Not sure how to interpret "going-to-be-deprecated" -- does that mean we actually did not deprecate this flag in 1.1 but just intend to do so at a later time?
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604547699
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
Some weird things I noticed: First, the ticket says `--execute` was deprecated in 1.1, but it doesn't even exist in 1.0 (or earlier AFAICT but I did not check all branches). Did we...add it and deprecate it in the same release? Huh?
Also, I just read [KIP-171](https://cwiki.apache.org/confluence/display/KAFKA/KIP-171+-+Extend+Consumer+Group+Reset+Offset+for+Stream+Application) and it's honestly super confusing. Regarding the deprecation, it says `"Current option '–execute' will be marked as "going-to-be-deprecated" and replaced by 'dry-run' parameter, until it gets replaced."`
Not sure how to interpret "going-to-be-deprecated" -- does that mean we actually did not deprecate this flag in 1.1 but just intend to do so at a later time?
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604515864
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +246,10 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
Review comment:
I think we would have a 3.1 and afterwards 4.0.
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r602799226
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +246,10 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
Review comment:
In contrast to the `--zookeeper` flag, there is no indicator that this option was deprecated... Do we feel confident to remove it? Seems we missed to update this one when implementing KIP-171.
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604521479
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
done
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604523228
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
I'm starting to feel more like we should just go ahead and remove the `--execute` flag as well, it's not like there was a very visible deprecation even of the `--zookeeper` flag, it wasn't logged if we used it and there wasn't any note in the upgrade guide that I can see. Plus it's not like it's a huge effort to migrate away from the `--execute`, you literally just need to drop it from wherever you're invoking the reset tool -- doesn't seem to be that bad if we just remove it and quote which KIP it was that deprecated it to show that it was indeed voted on and deprecated. Probably no one uses it these days anyway. Thoughts?
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r610838558
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
It's a long time ago... Cannot recall. Because it's many year back, might also not be worth to spent time digging into to to reconstruct 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman edited a comment on pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman edited a comment on pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#issuecomment-810653716
Some unrelated failures:
`kafka.clients.admin.KafkaAdminClientTest.testClientSideTimeoutAfterFailureToReceiveResponse()` (failed on all three builds)
`kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()`
`kafka.server.ListOffsetsRequestTest.testResponseIncludesLeaderEpoch()`
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604517220
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +247,13 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
+
+ // TODO: deprecated in 3.0 can be removed in a future release -- https://issues.apache.org/jira/browse/KAFKA-7606
+ executeOption = optionParser.accepts("execute", "This option is deprecated and will be removed in a future release; it is not required to specify this in order to execute the command");
Review comment:
We did not to do this for ZK flag, but should we also log/print if the flag is used?
--
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] ableegoldman commented on pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#issuecomment-810653716
Some unrelated failures:
`kafka.clients.admin.KafkaAdminClientTest.testClientSideTimeoutAfterFailureToReceiveResponse()` (failed on all three builds)
`kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()`
`kafka.server.ListOffsetsRequestTest.testResponseIncludesLeaderEpoch()`
Since this PR had been approved before the things I added and then reverted, I'm going to merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman merged pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman merged pull request #10411:
URL: https://github.com/apache/kafka/pull/10411
--
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] ableegoldman commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r603626513
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -247,14 +246,10 @@ private void parseArguments(final String[] args) {
.describedAs("file name");
forceOption = optionParser.accepts("force", "Force the removal of members of the consumer group (intended to remove stopped members if a long session timeout was used). " +
"Make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances.");
- executeOption = optionParser.accepts("execute", "Execute the command.");
Review comment:
Hmm...awkward. Yeah I think we should probably not remove it yet in that case. I'll add the deprecation note in this PR, and then we can remove it in 4.0 (is 4.0 coming after 3.0 or will we have 3.1?)
--
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] mjsax commented on a change in pull request #10411: KAFKA-7606: Remove deprecated options from StreamsResetter
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10411:
URL: https://github.com/apache/kafka/pull/10411#discussion_r604528038
##########
File path: core/src/main/scala/kafka/tools/StreamsResetter.java
##########
@@ -267,6 +266,10 @@ private void parseArguments(final String[] args) {
CommandLineUtils.printUsageAndDie(optionParser, e.getMessage());
}
+ if (options.has(executeOption)) {
+ System.out.println("The --execute flag has been deprecated and will be removed in a future release, you do not need to specify anything to execute the command");
Review comment:
`System.err.println` ? (just an idea)
--
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