[GitHub] [samza] mynameborat commented on a diff in pull request #1651: Migrate zkclient library

["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