MINOR: disable JmxTool in kafkatest console-consumer by default #7785

brianbushree · 2019-12-05T17:40:16Z

what

Provide the ability to optionally enable the JmxTool inside the kafkatest console-consumer

why

For tests that don't use these JMX metrics, we should allow users to disable this metrics collection.

Committer Checklist (excluded from commit message)

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

tests/kafkatest/services/monitor/jmx.py

brianbushree · 2019-12-10T01:11:25Z

@hachikuji can you take another look please?

is this what you were thinking?

tests/kafkatest/services/monitor/jmx.py

hachikuji

Thanks, left a couple minor suggestions.

tests/kafkatest/services/console_consumer.py

hachikuji

@brianbushree Thanks, left a couple minor comments. Just to confirm, have you verified system tests are still passing with these changes? No need to check them all, maybe just make sure some test case which uses has_partitions_assigned (for example) still works.

tests/kafkatest/services/console_consumer.py

brianbushree · 2020-01-06T22:11:41Z

looks like I overlooked implementing proper default values with regard to inferring jmx_enabled from jmx_object_names/jmx_attributes...

currently we initialize the jmx_object_names and jmx_attributes here

self.jmx_object_names = ["kafka.consumer:type=consumer-coordinator-metrics,client-id=%s" % self.client_id]
self.jmx_attributes = ["assigned-partitions"]

if jmx_object_names is none (guessing we do this because the default value depends on another parameter, client-id)

unfortunately this means using jmx_object_names=None implies that we should use the default value...

seems like we have two options:

stick with a simple jmx_enabled flag
change the behavior of jmx_object_names such that we can pass in a string format and always replace %s with the client-id

@hachikuji what do you think? did I miss any other options?

hachikuji · 2020-01-07T23:46:47Z

I think the only reason we use the jmx tool by default is for the purpose of ConsoleConsumer.has_partitions_assigned. The only usage of this as far as I can tell is in produce_consume_validate.py and does not seem to be contributing significantly. I am thinking we can just remove this function and let the jmx tool be disabled by default. What do you think?

brianbushree · 2020-01-08T21:32:49Z

Ya I agree completely. It makes sense to disable this by default since it seems there are only a few usages and makes this PR much more complicated. I'll remove this function and some of the associated logic.

brianbushree · 2020-01-09T00:56:04Z

tests/kafkatest/services/console_consumer.py

@@ -102,6 +100,10 @@ def __init__(self, context, num_nodes, kafka, topic, group_id="test-consumer-gro
        for node in self.nodes:
            node.version = version

+        self.jmx_tool = JmxTool(context, num_nodes=num_nodes,
+                                jmx_object_names=jmx_object_names, jmx_attributes=(jmx_attributes or []),
+                                root=ConsoleConsumer.PERSISTENT_ROOT)


@mumrah to your earlier point, I've tried to avoid special casing by always initializing a JmxTool regardless of whether it'll be used

@hachikuji however, now that this is in place, it seems like we should maybe just stick with mixin approach since we're always initializing this object anyways? (this way we can also avoid some of the clean_node changes required here)

with that being said, I do like the consumer.jmx_tool namespace isolation

@brianbushree I don't mind going back to the mixin if you think that's better. I'll leave it up to you.

I think this mixin approach is better for two reasons here:

it should minimize this PR

seems less like a one-off approach since we already use the mixin everywhere

I'm going to refactor this PR accordingly.

brianbushree · 2020-01-09T22:40:27Z

These are the tests that seem to rely on this jmx functionality in the consumer:

fetch_from_follower_test.py
streams_simple_benchmark_test.py
quota_test.py

I've tested these locally and they are passing (I also tested compression_test.py which does not rely on this jmx functionality)

hachikuji

LGTM.

Do not initialize `JmxTool` by default when running console consumer. In order to support this, we remove `has_partitions_assigned` and its only usage in an assertion inside `ProduceConsumeValidateTest`, which did not seem to contribute much to the validation. Reviewers: David Arthur <[email protected]>, Jason Gustafson <[email protected]>

what/why the throttling_test was broken by this PR (#7785) since it depends on the consumer having partitions-assigned before starting the producer this PR provides the ability to wait for partitions to be assigned in the console consumer before considering it started. caveat this does not support starting up the JmxTool inside the console-consumer for custom metrics while using this wait_until_partitions_assigned flag since the code assumes one JmxTool running per node. I think a proper fix for this would be to make JmxTool its own standalone single-node service alternatives we could use the EndToEnd test suite which uses the verifiable producer/consumer under the hood but I found that there were more changes necessary to get this working unfortunately (specifically doesn't seem like this test suite plays nicely with the ProducerPerformanceService) Reviewers: Mathew Wong <[email protected]>, Bill Bejeck <bbejeck.com>

…he#7785) Do not initialize `JmxTool` by default when running console consumer. In order to support this, we remove `has_partitions_assigned` and its only usage in an assertion inside `ProduceConsumeValidateTest`, which did not seem to contribute much to the validation. Reviewers: David Arthur <[email protected]>, Jason Gustafson <[email protected]>

) what/why the throttling_test was broken by this PR (apache#7785) since it depends on the consumer having partitions-assigned before starting the producer this PR provides the ability to wait for partitions to be assigned in the console consumer before considering it started. caveat this does not support starting up the JmxTool inside the console-consumer for custom metrics while using this wait_until_partitions_assigned flag since the code assumes one JmxTool running per node. I think a proper fix for this would be to make JmxTool its own standalone single-node service alternatives we could use the EndToEnd test suite which uses the verifiable producer/consumer under the hood but I found that there were more changes necessary to get this working unfortunately (specifically doesn't seem like this test suite plays nicely with the ProducerPerformanceService) Reviewers: Mathew Wong <[email protected]>, Bill Bejeck <bbejeck.com>

hachikuji requested a review from mumrah December 5, 2019 20:03

hachikuji reviewed Dec 6, 2019

View reviewed changes

tests/kafkatest/services/monitor/jmx.py Outdated Show resolved Hide resolved

brianbushree commented Dec 10, 2019

View reviewed changes

tests/kafkatest/services/monitor/jmx.py Outdated Show resolved Hide resolved

hachikuji reviewed Dec 16, 2019

View reviewed changes

tests/kafkatest/services/console_consumer.py Outdated Show resolved Hide resolved

tests/kafkatest/services/console_consumer.py Outdated Show resolved Hide resolved

hachikuji reviewed Jan 6, 2020

View reviewed changes

tests/kafkatest/services/console_consumer.py Outdated Show resolved Hide resolved

tests/kafkatest/services/console_consumer.py Outdated Show resolved Hide resolved

mumrah reviewed Jan 6, 2020

View reviewed changes

tests/kafkatest/services/console_consumer.py Outdated Show resolved Hide resolved

moving JmxTool class + disabling jmx by default in consumer class

a3677b9

brianbushree force-pushed the consumer-jmx-opt branch from b5e36b3 to a3677b9 Compare January 9, 2020 00:41

brianbushree changed the base branch from 2.4 to trunk January 9, 2020 00:42

brianbushree commented Jan 9, 2020

View reviewed changes

moving back to mixin approach

d1d19b4

brianbushree changed the title ~~MINOR: optionally enable JmxTool in kafkatest console-consumer~~ MINOR: disable JmxTool in kafkatest console-consumer by default Jan 10, 2020

hachikuji approved these changes Jan 10, 2020

View reviewed changes

hachikuji merged commit 422bc1f into apache:trunk Jan 10, 2020

brianbushree mentioned this pull request Feb 28, 2020

MINOR: add wait_for_assigned_partitions to console-consumer #8192

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: disable JmxTool in kafkatest console-consumer by default #7785

MINOR: disable JmxTool in kafkatest console-consumer by default #7785

brianbushree commented Dec 5, 2019

brianbushree commented Dec 10, 2019

hachikuji left a comment

hachikuji left a comment

brianbushree commented Jan 6, 2020

hachikuji commented Jan 7, 2020

brianbushree commented Jan 8, 2020

brianbushree Jan 9, 2020

hachikuji Jan 9, 2020

brianbushree Jan 9, 2020

brianbushree commented Jan 9, 2020 •

edited

Loading

hachikuji left a comment

MINOR: disable JmxTool in kafkatest console-consumer by default #7785

MINOR: disable JmxTool in kafkatest console-consumer by default #7785

Conversation

brianbushree commented Dec 5, 2019

what

why

Committer Checklist (excluded from commit message)

brianbushree commented Dec 10, 2019

hachikuji left a comment

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

brianbushree commented Jan 6, 2020

hachikuji commented Jan 7, 2020

brianbushree commented Jan 8, 2020

brianbushree Jan 9, 2020

Choose a reason for hiding this comment

hachikuji Jan 9, 2020

Choose a reason for hiding this comment

brianbushree Jan 9, 2020

Choose a reason for hiding this comment

brianbushree commented Jan 9, 2020 • edited Loading

hachikuji left a comment

Choose a reason for hiding this comment

brianbushree commented Jan 9, 2020 •

edited

Loading