-
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
check and warn for invalid partition weight #1033
Conversation
d2/src/main/java/com/linkedin/d2/balancer/servers/ZooKeeperAnnouncer.java
Outdated
Show resolved
Hide resolved
512fa07
to
6481440
Compare
d2/src/main/java/com/linkedin/d2/balancer/servers/ZooKeeperAnnouncer.java
Show resolved
Hide resolved
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.
Overall looks good, I had one doubt about the implementation logic and also wondering if we can add a test for validatePartitionData
.
Also, are we deciding not to emit a metric initially & instead rely on logs to detect use cases with invalid partition data? Or does emitting the metric not need any restli change?
Summary
Announcer weight has been a double with no limit rules. Technically, a maximum of 100, and at most 2 digits after the decimal point should be sufficient to serve all load distribution needs in reality. With INDIS observer serving Envoy clients which require an integer uri weight, unlimited double number will cause conversion complexity that should never be encountered in real use case. In this PR, we add a max weight config which defines the max value and the number of decimal places allowed. We also added another config for the action to take when the weight rule is breached. It can be configured to ignore, log warn msg, throw exception, or rectify the value.
Note that this change will warn invalid weights set dynamically thru ZookeeperAnnouncerJmx as well.
Also added jmx methods to return the count of weight breaches for max value and decimal places, and the current markUp status of the announcer for regular cluster and dark warmup cluster respectively
See container PR for LI-specific use case and tests.
Test Done
See container PR.