improve kafka client sensor registration performance by lazily calculating JMX attributes

[radai-rosenblatt]

kafka re-registers its sensor MBean on any sensor change (addition/removal of sensors).
kafka also has per-topic-partition sensors, and the mbean attribute array size is a multiple of those.
on large assignment sets (~35K topic partitions assigned to a single consumer), we've seen sensor registration code take 5 entire consecutive minutes (!!) of CPU time.

the offending code path is in (re)registering the MBean, which triggers this code (called by DefaultMBeanServerInterceptor.registerMBean())

    private static String getNewMBeanClassName(Object mbeanToRegister)
            throws NotCompliantMBeanException {
        if (mbeanToRegister instanceof DynamicMBean) {
            DynamicMBean mbean = (DynamicMBean) mbeanToRegister;
            final String name;
            try {
                name = mbean.getMBeanInfo().getClassName(); <----- THIS
            } catch (Exception e) {
                // Includes case where getMBeanInfo() returns null
                NotCompliantMBeanException ncmbe =
                    new NotCompliantMBeanException("Bad getMBeanInfo()");
                ncmbe.initCause(e);
                throw ncmbe;
            }
            if (name == null) {
                final String msg = "MBeanInfo has null class name";
                throw new NotCompliantMBeanException(msg);
            }
            return name;
        } else
            return mbeanToRegister.getClass().getName();
    }

this triggers the creation of the (big) attribute array, while the caller really only wants the mbean name.

this patch delays the array creation to the time when the mbean attributes are actually queried - which may be never (in case no one is even looking at the jmx sensors).

in local testing this removes a ~5 minute delay in rebalancing/assigning large groups of topic partitions.

[satishd]

LGTM.

MBeanInfo class does not access field values like attributes because arrayGetSafe will be false for subclasses.

[lindong28]

LGTM. It should be a safe optimization. @hachikuji do you think this patch is OK? Thanks!

[ijuma]

This caused a regression as it broke serialization #5114

[lindong28]

@ijuma I am sorry for the problem caused by this. And thanks much for noticing this. I missed the serialization issue when thinking about the safty of this patch.

If I remember correctly, at most one seemingly-unrelated test for one scala version failed for this test before this PR is merged. So it seems that the existing Kafka code does not attempt to serialize the MBeanInfo returned by KafkaMbean.getMBeanInfo()? Is the problem caught only when a library outside Kafka repository (e.g. KSQL) attempts serialize the MBeanInfo?

In order to prevent for this in the future, I will write a unit test for this issue after understanding where it caused the problem.

[ijuma]

@lindong28 Yes, I agree that our testing is lacking here and thanks for offering to improve that. The serialization happens automatically when other processes read JMX metrics via RMI. Some Confluent system tests failed as well as some internal deployments that rely on JMX metrics. I'm not sure why Kafka system tests didn't fail. There were a couple of failures in the last build, but I don't know if they are related:

http://confluent-kafka-system-test-results.s3-us-west-2.amazonaws.com/2018-05-22--001.1526992451--apache--trunk--70a506b/report.html

[lindong28]

@ijuma Yeah it is not clear whether these tests are related. It will be easier for us to debug those tests now that we have reverted this patch. We will need test for this patch later.

[lindong28]

@ijuma After digging into the root cause of this performance issue, I find that in https://issues.apache.org/jira/browse/KAFKA-4381 we introduced per-partition metrics where we put the topic partition in the metric name instead of the tags. This introduces mbean whose number of attributes is proportional to the number of topic partition of the consumer. This can significantly reduce the MM startup time (see #5011) because consumer needs to create O(n^2) number of objects for metrics during initialization, where n is the number of topic partition assigned to the consumer.

Fortunately, KIP-225 (https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=74686649) has deprecated these metrics and moved the topic partition information to the tags of the new metrics. Thus we already have a long term fix for this issue.

Radai has a solution to override the mbean class here but it requires non-trivial trivial. Since we already have a long term solution for the issue, we probably want to keep the code base as simple as possible.

I also have two other ideas for hot fixing the issue, i.e. use time-based or counter-based way to just return empty list of attribute when getMBeanInfo() if called right after the KafkaMbean is instantiated. The time-based solution is probably safe because the user/tool usually pull jmx metrics repeatedly and it seems OK to be delayed by e.g. 100 ms. Maybe Radai can test the performance again with KIP-225. We can revisit this if there is still issue after we remove those problematic metrics.

[radai-rosenblatt]

i fixed the jmx and serialization issues (with the help of SO).
as for testing - to fully recreate all the issues a test would need not just to serialize the objects - but do an RMI call from a different JVM with a "clean" classpath.

we will probably end up using this internally.

as @lindong28 has removed the offending sensors, and KIP-225 would probably end up trading tens of thousands of attributes for tens of thousands of MBeans, i'll leave the decision on whether or not to merge this up to you.

[ijuma]

Do you have performance numbers before and after with this change post KIP-225?

[radai-rosenblatt]

i dont. i'll try running my original workload with vanilla 1.1.0 next week (jira says kip-225 landed in 1.1.0, right?)

[lindong28]

Note that KIP-225 does not immediately address the performance issue. It deprecates the problematic metrics. We need to additionally remove the problematic metrics (see #5172) before checking whether this PR improves performance.

[radai-rosenblatt]

true, so i'll need to run against trunk.

[radai-rosenblatt]

closing this as after removing the old metrics this is (likely) not required