KAFKA-14586: Moving StreamResetter to tools by vamossagar12 · Pull Request #13127 · apache/kafka

vamossagar12 · 2023-01-18T17:42:48Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

vamossagar12 · 2023-01-18T17:44:11Z

@mjsax , I noticed you had worked on the StreamsResetter tool. Would you be able to review this smallish PR? The idea is to move it from core module to tools module which is part of this: https://issues.apache.org/jira/browse/KAFKA-14525

vamossagar12 · 2023-01-19T04:56:12Z

Adding @fvaleri . There are checkstyle issues, which I would fix.

cadonna

@vamossagar12 Thanks for the PR!

I had one major comment.

vamossagar12 · 2023-01-28T03:23:49Z

@vamossagar12 Thanks for the PR!

I had one major comment.

Thanks @cadonna . I think that comment has been addressed now based on the conversation.

vamossagar12 · 2023-02-08T09:52:08Z

@cadonna , bumping this thread again.

fvaleri · 2023-02-08T13:25:39Z

@vamossagar12 as you can see from the CI output, there are checkstyle errors. Have you tried to run at least one of the system tests that are using the tool?

vamossagar12 · 2023-02-08T15:42:31Z

@vamossagar12 as you can see from the CI output, there are checkstyle errors. Have you tried to run at least one of the system tests that are using the tool?

hi @fvaleri , yes I am aware of the checkstyle issue and pointed it out here: #13127 (comment). I have run the StreamResetter tests and also modified the system test.

cadonna · 2023-02-09T08:40:31Z

@vamossagar12 The PR LGTM except for the checkstyle issues. Could you also please post a link to a successful system test run of the system tests that use the streams resetter?

vamossagar12 · 2023-02-09T09:34:46Z

Thanks @cadonna , would do the same.

vamossagar12 · 2023-02-10T09:14:22Z

@cadonna , @fvaleri I ran the system test for StreamsResetter from my local (and fixed the checkstyle). The tests are running, would keep an eye on it =>

./gradlew clean systemTestLibs                                                                                         

> Configure project :
Starting build with version 3.5.0-SNAPSHOT (commit id af3bedfd) using Gradle 7.6, Java 1.8 and Scala 2.13.10
Build properties: maxParallelForks=8, maxScalacThreads=8, maxTestRetries=0

BUILD SUCCESSFUL in 5m 44s
220 actionable tasks: 212 executed, 8 up-to-date
(ducktape) sarao@C02GG1KCML7H kafka % TC_PATHS="tests/kafkatest/tests/streams/streams_optimized_test.py::StreamsOptimizedTest" bash tests/docker/run_tests.sh

> Configure project :
Starting build with version 3.5.0-SNAPSHOT (commit id af3bedfd) using Gradle 7.6, Java 1.8 and Scala 2.13.10
Build properties: maxParallelForks=8, maxScalacThreads=8, maxTestRetries=0

BUILD SUCCESSFUL in 9s
172 actionable tasks: 172 up-to-date
docker exec ducker01 bash -c "cd /opt/kafka-dev && ducktape --cluster-file /opt/kafka-dev/tests/docker/build/cluster.json  ./tests/kafkatest/tests/streams/streams_optimized_test.py::StreamsOptimizedTest "
/usr/local/lib/python3.9/dist-packages/paramiko/transport.py:236: CryptographyDeprecationWarning: Blowfish has been deprecated
  "class": algorithms.Blowfish,
[INFO:2023-02-10 01:05:18,393]: starting test run with session id 2023-02-10--002...
[INFO:2023-02-10 01:05:18,395]: running 1 tests...
[INFO:2023-02-10 01:05:18,398]: Triggering test 1 of 1...
[INFO:2023-02-10 01:05:18,454]: RunnerClient: Loading test {'directory': '/opt/kafka-dev/tests/kafkatest/tests/streams', 'file_name': 'streams_optimized_test.py', 'cls_name': 'StreamsOptimizedTest', 'method_name': 'test_upgrade_optimized_topology', 'injected_args': {'metadata_quorum': 'REMOTE_KRAFT'}}
[INFO:2023-02-10 01:05:18,480]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: on run 1/1
[INFO:2023-02-10 01:05:18,498]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Setting up...
[INFO:2023-02-10 01:05:18,503]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Running...
[INFO:2023-02-10 01:08:00,049]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Tearing down...
[INFO:2023-02-10 01:08:26,433]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: PASS
[INFO:2023-02-10 01:08:26,437]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Data: None
================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.11.3
session_id:       2023-02-10--002
run time:         3 minutes 8.101 seconds
tests run:        1
passed:           1
flaky:            0
failed:           0
ignored:          0
================================================================================
test_id:    kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT
status:     PASS
run time:   3 minutes 7.954 seconds
--------------------------------------------------------------------------------

vamossagar12 · 2023-02-10T11:13:36Z

Tests seemed to have passed for one of the builds. The tests that failed (even the one is Streams) seem unrelated to me.

fvaleri

Hi @vamossagar12, thanks for fixing the checkstyle errors.

I left a few comments. I also think that it would be better to use the shared CommandDefaultOptions class, like in all the other joptsimple tools (help and version code is duplicated here).

The system test looks good:

test_id:    kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT
status:     PASS
run time:   1 minute 12.791 seconds

fvaleri · 2023-02-12T16:07:49Z

This subpackage is not needed if you move StreamsResetterTest to tools test.

I guess this is related to my comment #13127 (comment)?

fvaleri · 2023-02-12T16:09:19Z

I think this test belongs to the tools module, where the class under test lives.

Yeah even I had thought so. But the original layout was also similar (i.e StreamsResetter was under tools and the test lived here) so I didn't change it. @cadonna , WDYT?

I would also move it to the tools module.
@mjsax @guozhangwang Was there a specific reason not to have the test in the same module as the StreamsResetter?

Got it @cadonna . Would wait for the response. Even I think it can move but let's see what guozhang and Mathias have to say.
Also, while we are at it, would like to know your opinion on #13127 (comment)

I would not wait for the answer. We can do that in a follow-up PR. Could you open a ticket for the move?

Thanks @cadonna , here's the follow up ticket: https://issues.apache.org/jira/browse/KAFKA-14851

vamossagar12 · 2023-02-13T10:00:41Z

Thanks for the review @fvaleri . I addressed/responded to the comments. Regarding moving to CommandDefaultOptions, is it better if we do it in a follow up PR as that is not directly connected to migrating to tools module. I am not saying it isn't important but trying to keep the scope of this PR limited to the migration since StreamsResetter has been working this way since the beginning. WDYT?

fvaleri · 2023-02-16T14:15:57Z

Regarding moving to CommandDefaultOptions, is it better if we do it in a follow up PR as that is not directly connected to migrating to tools module.

Hi @vamossagar12, I'm ok with doing it in a follow up PR. Do we have a Jira for that?

vamossagar12 · 2023-02-20T17:55:07Z

Regarding moving to CommandDefaultOptions, is it better if we do it in a follow up PR as that is not directly connected to migrating to tools module.

Hi @vamossagar12, I'm ok with doing it in a follow up PR. Do we have a Jira for that?

Thanks @fvaleri , here you go: https://issues.apache.org/jira/browse/KAFKA-14734

fvaleri

LGTM. Thanks!

vamossagar12 · 2023-02-23T10:24:46Z

Thanks @fvaleri !

vamossagar12 · 2023-03-06T15:38:24Z

@cadonna , bumping this one again :)

cadonna

Thanks @vamossagar12 !

LGTM!

cadonna · 2023-03-27T09:34:38Z

I restarted the build. Once the results are acceptable, I will merge the PR.

cadonna · 2023-03-28T12:40:28Z

Test failures are unrelated:

    Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.DedicatedMirrorIntegrationTest.testMultiNodeCluster()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.DedicatedMirrorIntegrationTest.testMultiNodeCluster()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testConnectorBoundary
    Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()

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>

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) Reviewers: Luke Chen <showuon@gmail.com>, Bruno Cadonna <cadonna@apache.org>, Sagar Rao <sagarmeansocean@gmail.com>

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>

cadonna added the streams label Jan 19, 2023

cadonna reviewed Jan 19, 2023

View reviewed changes

Comment thread build.gradle Outdated

vamossagar12 changed the title ~~Kafka 14586: Moving StreamResetter to tools~~ KAFKA-14586: Moving StreamResetter to tools Jan 24, 2023

vamossagar12 force-pushed the KAFKA-14586 branch from f850603 to fff2eaa Compare January 28, 2023 03:21

vamossagar12 force-pushed the KAFKA-14586 branch from f9497e9 to dda1072 Compare February 10, 2023 08:13

fvaleri suggested changes Feb 12, 2023

View reviewed changes

Sagar Rao added 5 commits February 13, 2023 15:18

initial commit

444e26c

KAFKA-14586: Moving StreamResetter to tools

d6b0295

Removing dependency of core from tools in build.gradle

b203b56

Fixing checkstyle issues

66f2438

Removing unwanted dependencies from build.gradle

680e971

vamossagar12 force-pushed the KAFKA-14586 branch from dda1072 to 680e971 Compare February 13, 2023 09:57

fvaleri approved these changes Feb 23, 2023

View reviewed changes

cadonna approved these changes Mar 27, 2023

View reviewed changes

cadonna merged commit c14f56b into apache:trunk Mar 28, 2023

fvaleri added the tools label Jul 7, 2023

fvaleri mentioned this pull request Jul 10, 2023

KAFKA-14734: Use CommandDefaultOptions in StreamsResetter #13983

Merged

Conversation

vamossagar12 commented Jan 18, 2023

Committer Checklist (excluded from commit message)

Uh oh!

vamossagar12 commented Jan 18, 2023

Uh oh!

vamossagar12 commented Jan 19, 2023

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vamossagar12 commented Jan 28, 2023

Uh oh!

vamossagar12 commented Feb 8, 2023

Uh oh!

fvaleri commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vamossagar12 commented Feb 8, 2023

Uh oh!

cadonna commented Feb 9, 2023

Uh oh!

vamossagar12 commented Feb 9, 2023

Uh oh!

vamossagar12 commented Feb 10, 2023

Uh oh!

vamossagar12 commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fvaleri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fvaleri Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

vamossagar12 Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

fvaleri Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

vamossagar12 Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

cadonna Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vamossagar12 Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

vamossagar12 Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

vamossagar12 commented Feb 13, 2023

Uh oh!

fvaleri commented Feb 16, 2023

Uh oh!

vamossagar12 commented Feb 20, 2023

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

vamossagar12 commented Feb 23, 2023

Uh oh!

vamossagar12 commented Mar 6, 2023

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

cadonna commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fvaleri commented Feb 8, 2023 •

edited

Loading

vamossagar12 commented Feb 10, 2023 •

edited

Loading

fvaleri left a comment •

edited

Loading

cadonna Feb 14, 2023 •

edited

Loading

vamossagar12 Feb 16, 2023 •

edited

Loading

cadonna commented Mar 27, 2023 •

edited

Loading