Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-14734: Use CommandDefaultOptions in StreamsResetter #13983

Merged
merged 2 commits into from Jul 20, 2023
Merged

KAFKA-14734: Use CommandDefaultOptions in StreamsResetter #13983

merged 2 commits into from Jul 20, 2023
+293 −254

Conversation

fvaleri
Copy link
Collaborator

@fvaleri fvaleri commented Jul 10, 2023

This PR adds CommandDefaultOptions usage like in the other joptsimple based tools. It also moves the associated unit test class from streams to tools module as discussed in #13127 (comment).

@fvaleri
KAFKA-14734: Use CommandDefaultOptions in StreamsResetter
129aca9 
This PR adds CommandDefaultOptions usage like in the other joptsimple based tools.
It also moves the associated unit test class from streams to tools module as discussed in
apache#13127 (comment).

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
showuon
showuon approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the patch!

checkstyle/import-control.xml Show resolved Hide resolved
@showuon
Copy link
Contributor

showuon commented Jul 18, 2023

@vamossagar12 , do you have time to have a look?

@vamossagar12
Copy link
Collaborator

@showuon , I will take a look today.

vamossagar12
vamossagar12 approved these changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@vamossagar12 vamossagar12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit changes. LGTM otherwise. Also, thanks for picking the tests migration!

tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java Outdated Show resolved Hide resolved
tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java Show resolved Hide resolved
tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java Show resolved Hide resolved
@fvaleri
Make inputTopicPartitions and timestamp final
d1c2ed3 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@showuon
Copy link
Contributor

showuon commented Jul 20, 2023

Failed tests are unrelated:

Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[1] Type=Raft-Combined, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.6-IV0, Security=PLAINTEXT
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testReplication()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 20 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()

@showuon showuon merged commit 334c41d into apache:trunk Jul 20, 2023
1 check failed
@fvaleri fvaleri deleted the streams-resetter-opts branch July 20, 2023 11:24
jeqo pushed a commit to jeqo/kafka that referenced this pull request Jul 21, 2023
@fvaleri @jeqo
KAFKA-14734: Use CommandDefaultOptions in StreamsResetter (apache#13983)
7dff594 
This PR adds CommandDefaultOptions usage like in the other joptsimple based tools. It also moves the associated unit test class from streams to tools module as discussed in apache#13127 (comment)

Reviewers:  Luke Chen <showuon@gmail.com>, Bruno Cadonna <cadonna@apache.org>, Sagar Rao <sagarmeansocean@gmail.com>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
@fvaleri
KAFKA-14734: Use CommandDefaultOptions in StreamsResetter (apache#13983)
05fb279 
This PR adds CommandDefaultOptions usage like in the other joptsimple based tools. It also moves the associated unit test class from streams to tools module as discussed in apache#13127 (comment)

Reviewers:  Luke Chen <showuon@gmail.com>, Bruno Cadonna <cadonna@apache.org>, Sagar Rao <sagarmeansocean@gmail.com>
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
@fvaleri @jeqo
KAFKA-14734: Use CommandDefaultOptions in StreamsResetter (apache#13983)
6e49bf4 
This PR adds CommandDefaultOptions usage like in the other joptsimple based tools. It also moves the associated unit test class from streams to tools module as discussed in apache#13127 (comment)

Reviewers:  Luke Chen <showuon@gmail.com>, Bruno Cadonna <cadonna@apache.org>, Sagar Rao <sagarmeansocean@gmail.com>
@mjsax
Copy link
Member

mjsax commented Oct 4, 2023

Just discovered this PR -- Seems it part of KIP-906, but the KIP does not cover the renaming of the method -- it's technically a breaking change...

Given that StreamsResetter is marked as @InterfaceStability.Unstable it might be ok without a deprecation phase of the old method, but the KIP should mention it. Can we get the KIP updated accordingly?

\cc @cadonna who commented on the other PR.

@fvaleri
Copy link
Collaborator Author

fvaleri commented Oct 5, 2023 •
edited

Hi @mjsax, I'm sorry if this caused any issue, but the intention was to give some some consistency to how these tools are structured, whenever possible. This is not part of KIP-906, but simply in the context of KAFKA-14525.

@cadonna
Copy link
Contributor

cadonna commented Oct 5, 2023 •
edited

@mjsax @fvaleri The javadocs of the StreamsResetter say:

This class is not part of public API. For backward compatibility, use the provided script in "bin/" instead of calling this class directly from your code.

See

kafka/tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java

Line 65 in 334c41d

* <strong>This class is not part of public API. For backward compatibility,

I think we do not need to do anything.

@cadonna cadonna mentioned this pull request Oct 5, 2023
Merged
@mjsax
Copy link
Member

mjsax commented Oct 5, 2023

Not a big deal -- as StreamsResetter is not public API, we don't need a deprecation period. I was just trying to say, because StreamsResetter is "semi-public" it might be nice to mention the renaming without deprecation period in the KIP, for documentation purpose. Just a suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cadonna cadonna cadonna left review comments

@vamossagar12 vamossagar12 vamossagar12 approved these changes

@showuon showuon showuon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@fvaleri @showuon @vamossagar12 @mjsax @cadonna