SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController #153

navina · 2017-05-01T21:10:00Z

Addresses the following:

Makes LeaderElectionListener to be explicitly registered by the caller
Removes debouncetimer dependency from ZkLeaderElector implementation
[Bug] onBecomeLeader was scheduling a task in timer under "OnBecomeLeader", when it should actually be the same as "OnProcessorChange". Otherwise, it will not cancel when there is a new OnProcessorChange event.
[Transient Test Failure] TestScheduleAfterDebounceTime tests were relying on timing controlled by sleep. Fixed it by using latch

…ller

…tener, SamzaContainerListener

…ener for ThreadJob

…lerImpl

…ZkLeaderElector implementation

…bCoordinator to use LeaderElector as a library w/o ZkControlelr in the middle

…der the same name

navina · 2017-05-03T22:41:29Z

@xinyuiscool @sborya Can you guys please review this PR?

…ore efficient

xinyuiscool

Looks great. Thanks for the cleanup.

xinyuiscool · 2017-05-05T16:41:09Z

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

@@ -84,7 +73,7 @@ private void timerOff(final String version, final Stat currentStatOfBarrierDone)
      zkUtils.getZkClient().writeData(barrierDonePath, BARRIER_TIMED_OUT, currentStatOfBarrierDone.getVersion());
    } catch (ZkBadVersionException e) {
      // Expected. failed to write, make sure the value is "DONE"
-      ///LOG.("Barrier timeout write failed");
+      ///LOGGER.("Barrier timeout write failed");


remove the commented-out logging.

sborya

Couple of small nits.

sborya · 2017-05-05T16:48:13Z

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

@@ -39,13 +39,9 @@
 * ZK based standalone app.
 */
 public class ScheduleAfterDebounceTime {
-  public static final Logger LOG = LoggerFactory.getLogger(ScheduleAfterDebounceTime.class);
+  public static final Logger LOGGER = LoggerFactory.getLogger(ScheduleAfterDebounceTime.class);


Samza is using LOG through out the code. I think we should keep it as LOG.

I don't think we have settled on what is the convention. I have seen both being used. So, I simply decided to keep it consistent within the zk package in core. If you really want this changed, I can do it.

sborya · 2017-05-05T16:48:23Z

samza-core/src/main/java/org/apache/samza/coordinator/LeaderElector.java

   */
-  void tryBecomeLeader(LeaderElectorListener leaderElectorListener);
+  void setLeaderElectorListener(LeaderElectorListener listener);


I am wondering, why having a listener as a part of the LE object is better then passing it to the tyBecomeLeader call?
Is there any advantage besides the semantic preferences?

It just makes the code more modularized and testable.

Aside from that, if you are passing some kind of state within a recursive call, you need to manage the lifecycle of that instance in recursive function. It makes the code more vulnerable to bugs.

Speaking of semantics, it is the same callback function that needs to be invoked in every call. So, there is really no need to pass around a listener that doesn't do anything different. Does this make sense?

sborya · 2017-05-05T16:50:04Z

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

@@ -36,44 +36,33 @@
 * This class creates a barrier for version upgrade.
 * Barrier is started by the participant responsible for the upgrade. (start())
 * Each participant will mark its readiness and register for a notification when the barrier is reached. (waitFor())
- * If a timer (started in start()) goes off before the barrier is reached, all the participants will unsubscribe
+ * If a timer (started in start()) goes off before the barrier is reached, all the participants will un-subscribe


eh, I think unsubscribe is the right form.

err.. yeah..I think I was blindly following IntelliJ's spelling suggestion. Thanks! I will fix it.

sborya · 2017-05-05T16:53:19Z

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

@@ -22,11 +22,18 @@

 /**
 * Api to the functionality provided by ZK
+ *
+ * Api for JC -&gt; ZK communication


please just write JC to ZK communication

sborya · 2017-05-05T17:03:05Z

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

@@ -59,7 +58,7 @@ public Latch getLatch(int size, String latchId) {
  public BarrierForVersionUpgrade getBarrier(String barrierId) {
    return new ZkBarrierForVersionUpgrade(barrierId, zkUtils, debounceTimer, zkConfig.getZkBarrierTimeoutMs());
  }
-  @VisibleForTesting
+


we should remember to get rid of this method completely.

Yeah. I tried to get rid of this method. But the ZkBarrierVersionUpgrade depends on not only the timers but also the zkutils. I didn't want to start refactoring those classes as well. Perhaps you can address it as a part of SAMZA-1128
If it makes sense to address this in that JIRA, let me know. I will add a TODO in the code as well.

sborya · 2017-05-05T17:06:57Z

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


    zkUtils.makeSurePersistentPathsExists(new String[]{keyBuilder.getProcessorsPath()});
  }

+  @VisibleForTesting


yeah, I wish we didn't have to do this. Don't have any suggestions at the moment now, but if you do... :)

It is fine to introduce constructors annotated with @VisibleForTesting. The goal is make it clear that it is not to be used in any case other than testing. I don't see any pressing reason for us to remove the constructor overload.

…bounceTimer This PR depends on PR #153 * Treats all errors in jobcoordinator as FATAL and shuts-down the streamprocessor * [Bug] Fixed bug reported in SAMZA-1241 * Introduced a callback to be associated with the timer (same callback for every Runnable failure) **TBD**: some more unit tests Author: Navina Ramesh <navina@apache.org> Reviewers: Xinyu Liu <xiliu@apache.org>, Prateek Maheshwari <pmaheshw@linkedin.com> Closes #166 from navina/SAMZA-1150

navina added 30 commits May 3, 2017 15:07

Remove containerId from SamzaContainer.apply

8996cb5

Removing onBecomingLeader for readability

6931cc6

Removing awaitStart

528086d

Added JobCoordinator Listener. Trying to remove Samzacontainer contro…

eb16378

…ller

Adding ProcessorErrorHandler, SamzaContainerStatus, JobCoordinatorLis…

4d21ee5

…tener, SamzaContainerListener

Documenting state transitions for SamzaContainer

38e3f40

adding some log lines in LocalContainerRunner

bfe1f17

Fixed integration test failures by throwing the exception in the list…

376f6ac

…ener for ThreadJob

Added a few tests in TestSamzaContainer

38271d6

Added test for sp.stop()

2df4666

Adding setContainerListener explicitly in SamzaContainer

6f6255e

Added documentation in JobCoordinator interface

53bb48c

Removed ProcessorErrorHandler

012eeff

Removing commetned out code

48cf739

Adding docs to JobCoordinatorListener

6bf2905

Added javadocs for SamzaContainerListener

db96e5d

Cleaning up StreamProcessor code and jobCoordinator docs

ca27a87

Fixing standaloneJobCoordinator

4420359

Adding null checks on processorListener

525ac15

Fixing ZkJobCoordinator

23a6f76

Removing unused variable in ZkJobCoordinator

22e5ddd

Adding javadoc to sp.stop and removing incorrect comment in ZkControl…

4a58005

…lerImpl

Fixing LocalContainerRunner

6c580d3

Minor changes to codestyle and docs

3a5d181

Adding IllegalContainerStateException

ee29c0f

Moving pkg private methods after public methods in StreamProcessor

d58d789

Fixing comments based on Prateek's suggestions

edfad1e

Changing RUNNING to STARTED

3d59fc6

Changing documentation formatting

f58caee

Handling IllegalContainerStateException in StreamProcessor.stop

51db441

navina added 8 commits May 3, 2017 15:07

Adding a pause interface to SamzaContainer

9baa071

Another documentation fix

a5d277b

Fixing javadoc formatting

4cf6020

Adding log in ThreadJob's onContainerFailed

fc0fdbe

LeaderElector should explicitly register a listener. Cleaning up the …

61eb3a5

…ZkLeaderElector implementation

Moving LeaderElectorListener out of ZkController and refactoring ZkJo…

f64d38a

…bCoordinator to use LeaderElector as a library w/o ZkControlelr in the middle

Adding some javadocs to ZkController and ZkControllerListener interface

4e5ddd1

[Bug-fix] OnBecomeLeader and OnProcessorChange should be queued up un…

7a76cf0

…der the same name

navina force-pushed the SAMZA-1251 branch from c599fd1 to 7a76cf0 Compare May 3, 2017 22:23

Merging with master

ee6ed9f

navina added 3 commits May 3, 2017 15:59

removing unnecessary cast

d6f7a1d

Merging with upstream

0393ecd

Remove debounce timer from ZkControllerImpl

ca4ce7c

navina changed the title ~~SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector~~ SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController May 5, 2017

Removing thread sleeps in TestScheduleAfterDebounceTimer to make it m…

cdef243

…ore efficient

navina mentioned this pull request May 5, 2017

SAMZA-1150 : Handling Error propagation between ZkJobCoordinator & DebounceTimer #166

Closed

xinyuiscool approved these changes May 5, 2017

View reviewed changes

sborya approved these changes May 5, 2017

View reviewed changes

navina added 2 commits May 5, 2017 12:07

Addressing feedback from Boris and Xinyu

669386b

Merge branch 'master' of https://github.com/navina/samza into SAMZA-1251

0e771d4

asfgit closed this in fb7aa73 May 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController #153

SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController #153

navina commented May 1, 2017 •

edited

navina commented May 3, 2017

xinyuiscool left a comment

xinyuiscool May 5, 2017

navina May 5, 2017

sborya left a comment

sborya May 5, 2017

navina May 5, 2017

sborya May 5, 2017

navina May 5, 2017

sborya May 5, 2017

navina May 5, 2017

sborya May 5, 2017

navina May 5, 2017

sborya May 5, 2017

navina May 5, 2017

sborya May 5, 2017

navina May 5, 2017

SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController #153

SAMZA-1251 - Remove DebounceTimer dependency from ZkLeaderElector & ZkController #153

Conversation

navina commented May 1, 2017 • edited

navina commented May 3, 2017

xinyuiscool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sborya 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

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 commented May 1, 2017 •

edited