-
Notifications
You must be signed in to change notification settings - Fork 546
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.
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
Dynamically switch jmx/sensor names based on dual read mode and source type #926
Conversation
7c3074f
to
828b52a
Compare
828b52a
to
e8b922c
Compare
@Mock | ||
JmxManager _jmxManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put these mocks into a fixture because direct mock member vars inside the class can't be used in a data provider. For example, when I create different D2ClientJmxManager with different params but use the same _jmxManager mock:
@DataProvider
public Object[][] dataProvider()
{
return new Object[][]
{
{new D2ClientJmxManager(..., _jmxManager)}
{new D2ClientJmxManager(..., _jmxManager, ...ZK, null)}
{new D2ClientJmxManager(..., _jmxManager, ....xDS, null)}
};
}
Using this data provider in a test method (such as testSetSimpleLBStateListenerUpdateClusterInfo
), the test will fail with errors saying the mock is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine by me, it's easier to pass in one object than 10 !
discussed with @PapaCharlie over slack, it's better to separate out the watchers for different property types into separate maps or member vars, so it's less error-prone for future devs to add watchers for new types of properties. I did so and also moved the watcher management logic to a separate manager class for cleanlyness. |
Background
Currently Jmx/sensors (load balancer, load balancer state, file stores, clusterInfo, serviceProperties, LoadBalancerStrategy, etc) are having the same name in ZK and xDS read flow. The current code intended to give D2ClientJmxManager.java a different prefix in xDS flow ("-xDS"), which had a bug in container and wasn't working. Even if worked, it would still break the jmx/sensors because depending on the dynamically changing dual read mode value, the primary source (ZK in OLD_LB_ONLY and DUAL_READ modes; xDS in NEW_LB_ONLY mode) should register the jmx/sensor names same as today, so that users can still monitor the same metrics. And, the secondary source should register different names to avoid conflicting with the primary.
Changes
Test Done
Under dual read or zk-only modes, the ZK source is primary, and xDS source registered the names with "-xDS":
Under observer-only mode, app log shows the primary names are unregistered and re-registered (done by xDS source), and the "-ZK" names are registered (by ZK source):
, and the jmx/sensor names also show the same:
Note: the "-xDS" names are left there (not unregistered), since they won't hurt during the dual read testing and will be cleaned when the app adopts xDS load balancer (by config, so dual read load balancer is not used at all) and redeploys.