MINOR: Add JmxTool wrapper scripts and redirection (KIP-906)

[fvaleri]

No description provided.

[fvaleri]

@dajac @mimaison last commit shows the reflection based solution. Let me know if this is fine. Thanks.

[ijuma]

Was a KIP submitted for this? Generally, if something ends up in the release notes due to user impact, it requires a KIP.

[dajac]

I am not aware of any KIP regarding the migration of the tools from core module to tools module. Having a small one for those problematic tools is not a bad idea.

For the context, package name of the tools has been changed during the migration. For tools with a script, the change is transparent. For tools without a script, the change will break things. Hence my question in this PR.

[ijuma]

@dajac well, that depends on whether it's considered public api. My position is that the only API is the script. If there is no script, it's not public API. How would users even know about the tools if they're not documented? Are the tools in question documented?

[dajac]

Yeah, it is definitely a gray area. I don’t think that those tools are documented in the Kafka documentation. Somehow, JmxTool is known and you can find references to it on Stackoverflow and in articles online.

I don’t have a strong opinion on this but thoughts that it is worth having a discussion. Being user friendly is also nice in general.

[dajac]

By the way, there is another case here: #13214.

[Hangleton]

Thanks David for your suggestion about a KIP, this looks appropriate indeed. And, this can be an opportunity to add documentations for these SPIs, too.

[fvaleri]

@ijuma @dajac I'm working on this KIP. Thanks for the feedback.

[Hangleton]

@fvaleri Is the KIP going to include the interface MessageReader? Happy to write one for it otherwise.

[fvaleri]

MessageReader

Not specifically, I was thinking more about proposing a policy (series of guidelines) to address the common compatibility issues we have seen while kicking off this migration.

[fvaleri]

@ijuma @dajac

https://cwiki.apache.org/confluence/display/KAFKA/KIP-906%3A+Tools+migration+guidelines

[fvaleri]

@dajac @ijuma can you please review the above small KIP?

The vote thread is already opened.
Thanks.

[fvaleri]

Hi @mimaison and @dajac, now that KIP-906 is accepted I would say this is ready for the final review. Thanks.

[Hangleton]

Thanks for the PR.

[fvaleri]

@mimaison added the missing wrapper scripts for this tool as specified in KIP-906 and changed the system tests to use it.

System test run after latest changes:

TC_PATHS="tests/kafkatest/tests/core/throttling_test.py::ThrottlingTest.test_throttled_reassignment" \
  _DUCKTAPE_OPTIONS='--parameters '\''{"bounce_brokers":"false"}'\' \
    bash tests/docker/run_tests.sh
docker exec ducker01 bash -c "cd /opt/kafka-dev && ducktape --cluster-file /opt/kafka-dev/tests/docker/build/cluster.json  ./tests/kafkatest/tests/core/throttling_test.py::ThrottlingTest.test_throttled_reassignment --parameters '{"bounce_brokers":"false"}'"
/usr/local/lib/python3.9/dist-packages/paramiko/transport.py:236: CryptographyDeprecationWarning: Blowfish has been deprecated
  "class": algorithms.Blowfish,
[INFO:2023-04-12 06:15:49,034]: starting test run with session id 2023-04-12--001...
[INFO:2023-04-12 06:15:49,034]: running 1 tests...
[INFO:2023-04-12 06:15:49,035]: Triggering test 1 of 1...
[INFO:2023-04-12 06:15:49,042]: RunnerClient: Loading test {'directory': '/opt/kafka-dev/tests/kafkatest/tests/core', 'file_name': 'throttling_test.py', 'cls_name': 'ThrottlingTest', 'method_name': 'test_throttled_reassignment', 'injected_args': {'bounce_brokers': 'false'}}
[INFO:2023-04-12 06:15:49,043]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: on run 1/1
[INFO:2023-04-12 06:15:49,045]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: Setting up...
[INFO:2023-04-12 06:15:52,005]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: Running...
[INFO:2023-04-12 06:20:30,334]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: Tearing down...
[WARNING - 2023-04-12 06:20:30,669 - service_registry - stop_all - lineno:53]: Error stopping service <ProducerPerformanceService-0-139894120584000: num_nodes: 1, nodes: ['ducker09']>: 1
[INFO:2023-04-12 06:21:29,909]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: PASS
[INFO:2023-04-12 06:21:29,909]: RunnerClient: kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false: Data: None
================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.11.3
session_id:       2023-04-12--001
run time:         5 minutes 40.891 seconds
tests run:        1
passed:           1
flaky:            0
failed:           0
ignored:          0
================================================================================
test_id:    kafkatest.tests.core.throttling_test.ThrottlingTest.test_throttled_reassignment.bounce_brokers=false
status:     PASS
run time:   5 minutes 40.865 seconds
--------------------------------------------------------------------------------

[mimaison]

Technically KIP-906 was voted after KIP freeze but since some tools have already been migrated I think we should merge PRs that add the redirection and the warning message to avoid breaking things.

[mimaison]

@dajac Any objections?

[dajac]

@mimaison No objections. I do agree that we should avoid breaking things.

[mimaison]

LGTM

[mimaison]

Since the initial JmxTool changes are in 3.5, I've backported this to 3.5 too.