Replace 101tec ZkClient with Helix ZkClient #870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use helix version 1.0.2. Extend common/zk/ZkClient.java from helix zookeeper client and refactoring some code.
jzakaryan
reviewed
Nov 18, 2021
datastream-utils/src/main/java/com/linkedin/datastream/common/zk/ZkClient.java
Show resolved
Hide resolved
vmaheshw
reviewed
Nov 18, 2021
datastream-utils/src/main/java/com/linkedin/datastream/common/zk/ZkClient.java
Show resolved
Hide resolved
…on is unavailable. (linkedin#871) * Use topic level throughput information when partition level information is unavailable * Refactor code Co-authored-by: Vaibhav Maheshwari <vmaheshw@vmaheshw-mn1.linkedin.biz>
Co-authored-by: Vaibhav Maheshwari <vmaheshw@vmaheshw-mn1.linkedin.biz>
vmaheshw
reviewed
Dec 2, 2021
datastream-utils/src/main/java/com/linkedin/datastream/common/zk/ZkClient.java
Outdated
Show resolved
Hide resolved
datastream-utils/src/main/java/com/linkedin/datastream/common/zk/ZkClient.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Get all children of zk path. Changes the access modified to public, its defined as protected in parent class. | ||
| */ | ||
| @Override | ||
| public List<String> getChildren(final String path, final boolean watch) { |
There was a problem hiding this comment.
You can probably get rid of this method itself since you are only calling super.()
There was a problem hiding this comment.
The method is declared as protected in parent class and here I am just expanding the access specifier.
For now I thought to keep this around and later I can investigate where all this is used (across all MPs) and then see if it can be changed to use just the "public" getChildren method in parent class. Same for a few other methods that I have done this way.
| @Override | ||
| public String create(final String path, Object data, final CreateMode mode) throws RuntimeException { | ||
| if (path == null) { | ||
| throw new IllegalArgumentException("path must not be null."); |
There was a problem hiding this comment.
The new library throws NullPointerException instead of IllegalArgumentException. Can you plz check the contract of new methods ? Mostly the unit-tests are not covering it, otherwise the tests should have failed. May be a good idea to add those tests
There was a problem hiding this comment.
Also, this is something to bring up with Helix team that their contact says IllegalArgumentException but internally the method throws NullPointerException.
There was a problem hiding this comment.
Added unit test for NullPointerException. Will bring up the discrepancy with Helix team separately
…ze partitions (linkedin#876) The load based estimator is considering the topic level metrics, but the assigner code is not using the information to distribute the partitions. Fixing the code to derive the partition level metrics.
…ets (linkedin#873) * Flushless producer without comparing offsets * Flushless Producer Supporting Both Comparable and Non Comparable Offsets for Ack-ing * Fix comments * Making methods abstract * Created config support to use both strategies and added tests * Address comments * Address comments Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
Co-authored-by: Vaibhav Maheshwari <vmaheshw@vmaheshw-mn1.linkedin.biz>
…linkedin#882) * Fixed issue with missing exception message during task initialization * Changed log level to error
* Kafka upgrade * Kafka upgrade * Removing temp version * Addressing comments: removing flag update after close to prevent threads misbehavior * Removing dependency on li-apache-kafka-clients
* Skip onpartitionsrevoked during consumer.close() * Bumping up version * Fixing warning messages * Fixing warning messages
Co-authored-by: Vaibhav Maheshwari <vmaheshw@vmaheshw-mn1.linkedin.biz>
… time (linkedin#897) Previously when all duplicate streams expire at the same time (i.e. all duplicate streams might have expired but were waiting for a DS add/delete event) we incorrectly identified that a duplicate stream is still active since its still present in zk.
Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
…improvements (linkedin#887) Fixed bug where min/max partitions per task was being reported incorrectly. Previously, the min/max were the same across all datastreams. New setGauge method in DynamicMetricsManager enables explicitly setting a gauge value. This reduces some lifecycle management complexity in LoadBalancedPartitionAssigner. Incidental code quality improvements. Tests added to verify affected metrics (min/maxPartitionsPerTask). Fixed bug related to registration of Gauges and Timers (they weren't actually registered before).
* fix rebalancing-tasks bug and added tests * add some more test cases with even number of instances and with single instance Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
Remove minimally used Scala Dependencies from Brooklin code to avoid cross-scala versions builds.
Introduce braodcast API to TrasnportProvider This API will allow to sending events to all consumers/clients of TrasnsportProvider. TransportProvider will be able to optionally implement broadcast API. Example, for Kafka TrasnportProvider broadcast will send the event to all topic partitions.
* Update assignment to prevent duplicate task names * Address feedback and add test for same name but different fields * Simplify code since there cannot be duplicate datastream groups
* Fix Stopping Logic and Maintain Stopping Latch Counter * --amend Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
…fter merging linkedin#877 (linkedin#908) Co-authored-by: Shrinand Thakkar <sthakkar@sthakkar-mn1.linkedin.biz>
Use helix version 1.0.2. Extend common/zk/ZkClient.java from helix zookeeper client and refactoring some code.
Zookeeper version upgrade is not required.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use helix version 1.0.2. Extend common/zk/ZkClient.java from helix zookeeper client and refactoring
some code.
Important: DO NOT REPORT SECURITY ISSUES DIRECTLY ON GITHUB.
For reporting security issues and contributing security fixes,
please, email security@linkedin.com instead, as described in
the contribution guidelines.
Please, take a minute to review the contribution guidelines at:
https://github.com/linkedin/Brooklin/blob/master/CONTRIBUTING.md