You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by "ryucc (via GitHub)" <gi...@apache.org> on 2023/01/25 19:27:22 UTC
[GitHub] [samza] ryucc opened a new pull request, #1651: Migrate zkclient library
ryucc opened a new pull request, #1651:
URL: https://github.com/apache/samza/pull/1651
I0Itec zkclient has not been maintained for a while. Moving to the apache helix copy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a diff in pull request #1651: Migrate zkclient library
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088383350
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
We may need to just validate this internally before rolling this out. Maybe add a note in the PR description about potential degradation as this isn't tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat merged pull request #1651: [SAMZA-2773] Migrate zkclient library
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat merged PR #1651:
URL: https://github.com/apache/samza/pull/1651
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088226536
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
@mynameborat
The path forward isn't clear to me.
1. We want to remove the `@NonNull` line
2. Do we want to remove this constructor?
3. Need to choose one of the following:
a. Pass in a non-null ZkDataListener in the tests, or
b. Use the constructor without the last argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a diff in pull request #1651: Migrate zkclient library
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087260223
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
The annotation doesn't do much and is mostly for readability. Moreover, the annotation used in this from guava and I highly doubt anything is enforced through the annotation.
That said, if there are no usages in the code, you can remove this constructor. I am leaning towards making no change and have the tests use the same constructor since the behavior of two constructors differ slightly with validation on zkUtils.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088228074
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
Without this change, the unit test just fail with NPE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a diff in pull request #1651: Migrate zkclient library
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087260937
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
Since its used within the tests, i want to check if we are losing on the test coverage of testing the higher scale of the client (if it was intentional)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087123297
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
Another way to do this to add a null check, and call `ZkLeaderElector(String processorIdStr ZkUtils zkUtils)` if null.
There are no internal callers that pass in null here, except our unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087124399
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
Just noticed this method is annotationed `@VisibleForTesting` think it should be safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a diff in pull request #1651: Migrate zkclient library
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087109786
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkLeaderElector.java:
##########
@@ -409,12 +409,12 @@ public void testAmILeader() {
// Processor-1
ZkUtils zkUtils1 = getZkUtilsWithNewClient();
- ZkLeaderElector leaderElector1 = new ZkLeaderElector("1", zkUtils1, null);
+ ZkLeaderElector leaderElector1 = new ZkLeaderElector("1", zkUtils1);
Review Comment:
Uses a new constructor based on the change which includes validation of paths on zkutils which wasn't done previously.
Is that a concern for tests in terms of mocking additional behavior?
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
Was that change to 1024*1000 inside LinkedIn? If so, we should keep this `1024*1024*10` in OSS if helix client accepts this value.
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
Do we have any other callers for this method? Given you have modified the test caller to use different constructor? I want to make sure this non-null change is necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] sborya commented on a diff in pull request #1651: Migrate zkclient library
Posted by "sborya (via GitHub)" <gi...@apache.org>.
sborya commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087204826
##########
build.gradle:
##########
@@ -220,11 +220,12 @@ project(":samza-core_$scalaSuffix") {
dependencies {
compile project(':samza-api')
- compile("com.101tec:zkclient:$zkClientVersion") {
+ compile("org.apache.helix:zookeeper-api:1.1.0") {
Review Comment:
Seems like the version is usually supplied as a variable and is set in a separate file. Should we keep the style here too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087130547
##########
samza-core/src/main/java/org/apache/samza/zk/ZkLeaderElector.java:
##########
@@ -69,6 +71,7 @@ public ZkLeaderElector(String processorIdStr, ZkUtils zkUtils) {
@VisibleForTesting
public ZkLeaderElector(String processorIdStr,
ZkUtils zkUtils,
+ @Nonnull
IZkDataListener previousProcessorChangeListener) {
this.processorIdStr = processorIdStr;
Review Comment:
https://developer.android.com/reference/androidx/annotation/VisibleForTesting
Confirm this should be safe. This annotation would make the method `private` by default, so it is only used by our tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ajothomas commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ajothomas (via GitHub)" <gi...@apache.org>.
ajothomas commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087245610
##########
build.gradle:
##########
@@ -220,11 +220,12 @@ project(":samza-core_$scalaSuffix") {
dependencies {
compile project(':samza-api')
- compile("com.101tec:zkclient:$zkClientVersion") {
+ compile("org.apache.helix:zookeeper-api:1.1.0") {
Review Comment:
Also, looks like the version is use was 0.11
https://github.com/apache/samza/blob/master/gradle/dependency-versions.gradle#L49
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ajothomas commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ajothomas (via GitHub)" <gi...@apache.org>.
ajothomas commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087245610
##########
build.gradle:
##########
@@ -220,11 +220,12 @@ project(":samza-core_$scalaSuffix") {
dependencies {
compile project(':samza-api')
- compile("com.101tec:zkclient:$zkClientVersion") {
+ compile("org.apache.helix:zookeeper-api:1.1.0") {
Review Comment:
Also, looks like the version in use was 0.11
https://github.com/apache/samza/blob/master/gradle/dependency-versions.gradle#L49
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1087131768
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
The change was not inside LinkedIn, though they provide a way to override it. This is probably the most concerning change on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088181718
##########
build.gradle:
##########
@@ -220,11 +220,12 @@ project(":samza-core_$scalaSuffix") {
dependencies {
compile project(':samza-api')
- compile("com.101tec:zkclient:$zkClientVersion") {
+ compile("org.apache.helix:zookeeper-api:1.1.0") {
Review Comment:
@sborya I'll follow the same style.
@ajothomas the versioning might be different in the 2 libraries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] ryucc commented on a diff in pull request #1651: Migrate zkclient library
Posted by "ryucc (via GitHub)" <gi...@apache.org>.
ryucc commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088228978
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
@mynameborat what's the path forward here? Can we use version bumps?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] sborya commented on a diff in pull request #1651: Migrate zkclient library
Posted by "sborya (via GitHub)" <gi...@apache.org>.
sborya commented on code in PR #1651:
URL: https://github.com/apache/samza/pull/1651#discussion_r1088149860
##########
samza-core/src/test/java/org/apache/samza/zk/TestZkMetadataStore.java:
##########
@@ -41,7 +41,13 @@ public class TestZkMetadataStore {
private static final Random RANDOM = new Random();
- private static final int VALUE_SIZE_IN_BYTES = 1024 * 1024 * 10; // 10 MB
+ // In 101tec version, this constant was 1024 * 1024 * 10
+ // The limit changed to 1024 * 1000 after helix migration.
+ // A larger number would throw an exception.
+ // See: https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L2386
+ // The limit can be set through a system property, but not in unit tests.
+ // https://github.com/apache/helix/blob/654636e54268907deb2e12d32913455cc543b436/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java#L106
+ private static final int VALUE_SIZE_IN_BYTES = 512000;
Review Comment:
Why is it concerning? Isn't it used for test only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org