SAMZA-1165. cleanup old zk versions. #239

sborya · 2017-06-30T01:11:22Z

No description provided.

shanthoosh · 2017-06-30T18:24:22Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+  }
+
+  public void deleteOldVersionPath(String path, List<String> zNodeIds, int numVersionsToLeave, Comparator<String> c) {
+    if (StringUtils.isEmpty(path) || zNodeIds == null) {


Please add tests for all the delete methods?

added two tests.

shanthoosh · 2017-06-30T18:26:04Z

samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java

@@ -219,6 +219,8 @@ public void onNewJobModelConfirmed(String version) {
    if (coordinatorListener != null) {
      coordinatorListener.onNewJobModel(processorId, jobModel);
    }
+
+    zkUtils.cleanupZK(10);


Two questions here:
A) Can we please extract this 10 to a static constant in this class (Like NUM_CLEANUP_VERSIONS).

B) Is this getting called from leader alone or in all processors ? Two benefits doing it in leader

Will help us avert potential concurrency problems with delete.

Will remove additional overhead issuing delete in all stream processors when receiving this event.

shanthoosh · 2017-06-30T18:29:53Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+
+  public void deleteOldVersionPath(String path, List<String> zNodeIds, int numVersionsToLeave, Comparator<String> c) {
+    if (StringUtils.isEmpty(path) || zNodeIds == null) {
+      LOG.warn("cannot cleanup empty path or empty list in ZK");


Right behavior would be return or throw up exception(may be you forgot).

right. Good catch. Returned.

shanthoosh · 2017-06-30T18:37:45Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+      LOG.info("starting cleaning of barrier versions. size=" + size + "; num to leave=" +  numVersionsToLeave);
+      for (String znodeId: zNodeIds) {
+        i++;
+        if (size - i < numVersionsToLeave) {


Minor if it makes sense please change it:
Just wondering if we could simplify this check.

We could select numVersions from beginning of zNodeIds with subList or subArray (since zNodeIds.size() > numVersion) and delete this check completely. If this suggestion makes sense and acceptable, please change it.

shanthoosh · 2017-06-30T18:39:32Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+        LOG.info(path + "/" + znodeId);
+        zkClient.deleteRecursive(path + "/" + znodeId);
+      }
+


Minor if makes sense please change it:
It would be great to Log a debug statement here in else saying we're not cleaning up since (zNodeIds.size() <= numVersion)

may be too much. I added a logging of what we are deleting and how many we leave in place.

shanthoosh · 2017-06-30T18:40:43Z

samza-core/src/main/java/org/apache/samza/zk/ZkBarrierForVersionUpgrade.java

@@ -214,4 +212,7 @@ String getBarrierStatePath(String version) {
    }
  }

+  static public int getVersion(String barrierPath) {


Minor suggestion:

s/static public/public static

shanthoosh · 2017-06-30T18:48:24Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+  private void deleteOldBarrierVersions(int numVersionsToLeave) {
+    // read current list of barriers
+    String path = keyBuilder.getJobModelVersionBarrierPrefix();
+    LOG.info("jm path=" + path);


s/jm path/barrier path

shanthoosh · 2017-06-30T18:49:23Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+    deleteOldVersionPath(path, znodeIds, numVersionsToLeave,  new Comparator<String>() {
+      @Override
+      public int compare(String o1, String o2) {
+        // jm version name format is <num>


Jobmodel name format comment is in barrier method and Barrier name format comment is in jobmodel method. Please fix it.

shanthoosh · 2017-06-30T20:24:02Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+        if (size - i < numVersionsToLeave) {
+          break;
+        }
+        LOG.info(path + "/" + znodeId);


Please change description to deleting zk path since it's old or something like that.

Just logging path without context of taken action might not be useful.

shanthoosh · 2017-06-30T20:26:40Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+        }
+        LOG.info(path + "/" + znodeId);
+        zkClient.deleteRecursive(path + "/" + znodeId);
+      }


Minor if it makes sense please change it:
It would help to record metrics here when we delete zookeeper nodes. Will aid debugging.

I don't think we need metrics for cleanup. I think logging should be sufficient. Will add log.

…nto zkCleanUpBarrier1

shanthoosh

LGTM, have few minor suggestions. Please land after fixing them.

Thanks.

shanthoosh · 2017-07-19T00:59:32Z

samza-core/src/test/java/org/apache/samza/zk/TestZkUtils.java

@@ -237,6 +240,110 @@ public void testPublishNewJobModel() {
    Assert.assertEquals(jobModel, zkUtils.getJobModel(version));
  }

+  @Test
+  public void testClenaupZkJobModels() {


s/testClenaupZkJobModels/testCleanUpZkJobModels

shanthoosh · 2017-07-19T01:02:27Z

samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java

@@ -60,6 +60,7 @@
  // TODO: MetadataCache timeout has to be 0 for the leader so that it can always have the latest information associated
  // with locality. Since host-affinity is not yet implemented, this can be fixed as part of SAMZA-1197
  private static final int METADATA_CACHE_TTL_MS = 5000;
+  public static final int NUM_VERSIONS_TO_LEAVE = 10;


Can we please make this private(No usages outside).

shanthoosh · 2017-07-19T01:05:08Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+    String path = keyBuilder.getJobModelVersionBarrierPrefix();
+    LOG.info("about to delete old barrier paths from " + path);
+    List<String> znodeIds = zkClient.getChildren(path);
+    LOG.info("all ids are ids = " + znodeIds);


Could be better to have clear log message("list of zNodeIds " + zNodeIds).

shanthoosh · 2017-07-19T01:06:53Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+      // get the znodes to delete
+      int size = zNodeIds.size();
+      List<String> zNodesToDelete = zNodeIds.subList(0, numVersionsToLeave);
+      LOG.info("starting cleaning of barrier versions. from size=" + size + "to size " + zNodesToDelete.size() + "; num to leave=" + numVersionsToLeave);


s/starting cleaning of barrier versions/starting cleanUp of zKNodes.
This method is shared between deleteJobModel, deleteBarrier (Specific log message to barrier is not valid).

navina

couple of nits. Otherwise, looks good! +1

navina · 2017-07-19T21:28:31Z

samza-core/src/main/java/org/apache/samza/zk/ZkUtils.java

+      LOG.info("Starting cleanup of barrier version zkNodes. From size=" + size + " to size " + zNodesToDelete.size() + "; num to leave=" + numVersionsToLeave);
+      for (String znodeId : zNodesToDelete) {
+        String pathToDelete = path + "/" + znodeId;
+        LOG.info(pathToDelete);


nit: redundant log line. Line 516 conveys the same info. We can remove this.

Right! Removed.

navina · 2017-07-19T21:32:19Z

samza-core/src/main/java/org/apache/samza/zk/ScheduleAfterDebounceTime.java

@@ -48,6 +48,8 @@
  // Action name when the Processor membership changes
  public static final String ON_PROCESSOR_CHANGE = "OnProcessorChange";

+  public static final String ON_ZK_CLEANUP = "OnCleanUp";


nit: can we document about this cleanup policy here?

Author: Boris Shkolnik <boryas@apache.org> Author: Boris Shkolnik <bshkolni@linkedin.com> Reviewers: Navina <navina@apache.org>, Shanthoosh V<svenkata@linkedin.com> Closes apache#239 from sborya/zkCleanUpBarrier1

sborya added 2 commits June 29, 2017 18:08

cleanup old zk versions

754b29c

checkstyle

6a0c203

shanthoosh reviewed Jun 30, 2017

View reviewed changes

sborya and others added 17 commits June 30, 2017 17:00

more checkStyle

9b9accf

checkstyle

bc00451

review comments

35d9f53

merge

1359a2e

merge

58242a1

wait excplicitly for reading events instead of wait

5b6020f

Merge branch 'zkCleanUpBarrier1' of https://github.com/sborya/samza i…

6db29cc

…nto zkCleanUpBarrier1

checkstyle

297c620

added tests

324a51d

Merge branch 'zkCleanUpBarrier1' of https://github.com/sborya/samza i…

3e1e737

…nto zkCleanUpBarrier1

checkStyle

6a02c51

merge

563aa90

fix test

c042f79

test fix

b3aad6d

set container shutdown timeout for the tests

429c1ad

set container shutdown timeout for the tests

72a2cd5

extra space

4f43678

shanthoosh approved these changes Jul 19, 2017

View reviewed changes

make cleanup async

2c452d6

navina approved these changes Jul 19, 2017

View reviewed changes

sborya added 7 commits July 20, 2017 10:46

review comments

c5db2fe

@see comment

f8c6bd9

offset by one, for sublist

084cfe0

use different ids in the tests

66d4320

num to remove = total - num_to_leave

9ae3b5a

comments

1910ba0

give a longer time for processor restart

e59a712

asfgit closed this in 4eb5153 Jul 21, 2017

sborya deleted the zkCleanUpBarrier1 branch October 25, 2017 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAMZA-1165. cleanup old zk versions. #239

SAMZA-1165. cleanup old zk versions. #239

sborya commented Jun 30, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh Jun 30, 2017 •

edited

sborya Jul 14, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh Jun 30, 2017

sborya Jul 14, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh Jun 30, 2017

sborya Jul 14, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh Jun 30, 2017

sborya Jul 14, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh Jun 30, 2017

sborya Jul 17, 2017

shanthoosh left a comment

shanthoosh Jul 19, 2017

sborya Jul 19, 2017

shanthoosh Jul 19, 2017

sborya Jul 19, 2017

shanthoosh Jul 19, 2017

sborya Jul 19, 2017

shanthoosh Jul 19, 2017

sborya Jul 19, 2017

navina left a comment

navina Jul 19, 2017

sborya Jul 20, 2017

navina Jul 19, 2017

sborya Jul 20, 2017

SAMZA-1165. cleanup old zk versions. #239

SAMZA-1165. cleanup old zk versions. #239

Conversation

sborya commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanthoosh Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanthoosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanthoosh Jun 30, 2017 •

edited