SAMZA-1212 - Refactor interaction between StreamProcessor, JobCoordinator and SamzaContainer #148

navina · 2017-04-28T07:40:34Z

See SAMZA-1212 for motivation toward this refactoring.
Changes here are:

Removed awaitStart (blocking) method in StreamProcessor, JobCoordinator and SamzaContainer
Introduced SamzaContainerListener and JobCoordinatorListener interface implemented by StreamProcessor
Introduced SamzaContainerStatus to handler failures and lifecycle using Listener interfaces

navina · 2017-04-28T07:41:14Z

@prateekm @xinyuiscool Can you please review?

Note I am still working on adding unit tests for StreamProecssor. Just wanted to get the review started as the changes are quite significant. Thanks!

fredji97 · 2017-04-28T23:21:43Z

samza-core/src/main/java/org/apache/samza/SamzaContainerStatus.java

+  STOPPED,
+
+  /**
+   * Indicates that the container failed during any of its 3 active states -


In the above diagram, "FAILED" does not come from "STOPPED"

Yes. It is expected to be in either on of these states. I "failed" job cannot transition to "stopped". These are 2 different End states of the container.

fredji97 · 2017-04-29T00:24:18Z

samza-core/src/main/java/org/apache/samza/processor/JobCoordinatorListener.java

+   *   Method invoked by a {@link org.apache.samza.coordinator.JobCoordinator} when it is shutting without any errors
+   * </p>
+   */
+  void onCoordinatorStop();


Since this is just JobCoordinatorListener, would it be cleaner to just call this and the following one onStop() and onFailure()?

May be. It was getting pretty confusing when I initially started between the 3 groups of listeners - container listener, coordinator listener and processorListener, and each of them had the same onStart and onStop apis :) That's why I decided to keep it more explicitly in this case.

fredji97 · 2017-04-29T00:24:57Z

samza-core/src/main/java/org/apache/samza/processor/SamzaContainerListener.java

+   *   {@link org.apache.samza.container.RunLoop}
+   * </p>
+   */
+  void onContainerStart();


would it be cleaner to call it onStart()? And also onStop() and onFailure for the following ones?

Same explanation as above :)

fredji97 · 2017-04-29T01:35:22Z

samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java

+
+      @Override
+      public void onJobModelExpired() {
+        if (container != null && container.getStatus().equals(SamzaContainerStatus.RUNNING)) {


Better off to do SamzaContainerStatus.RUNNING.equals(container.getStatus()) to avoid check if container.getStatus() is null.

Good catch. Will fix it.

If container's not null but not running (hypothetical example: jobModelUpdate followed immediately by a jobModelExpired before container transitioned to running), shouldn't we try to stop and replace it regardless? Might be safer to remove this check and log the container status if not RUNNING in the body instead.

Also, Maybe log in else that expired was called but container was null?

shouldn't we try to stop and replace it regardless?

Fair point about container not having transitioned to RUNNING state.

Might be safer to remove this check and log the container status if not RUNNING in the body instead.

I don't understand what you mean by log the container status in the body?? Are you suggesting that we invoke shutdown, irrespective of the state of the container?

Just realized we still need the state check since calling shutdown on a stopped/failed container won't countdown the latch.

I think we can change this check to if (container != null && container != stopped or failed)
That way if it's not started yet, it'll go to RUNNING -> STOPPED immediately. Does that sound reasonable?

As an aside, we should throw in SamzaContainer if run is called on a STOPPED | FAILED container.

I think we can change this check to if (container != null && container != stopped or failed)

Yeah. Sounds good. Negative test condition works better here.

As an aside, we should throw in SamzaContainer if run is called on a STOPPED | FAILED container.

Ok. Let me try adding these checks. Thanks!

fredji97 · 2017-05-01T21:36:19Z

samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java

+  public synchronized void stop() {
+    if (container != null) {
+      LOGGER.info("Shutting down container " + container.toString() + " from StreamProcessor");
+      container.shutdown(false);


besides container.shutdown, should we do jobcoordinator stop as well?

Container shutdown should automatically trigger onContainerStop callback that will trigger the JobCoordinator to stop. There are multiple ways in which the StreamProcessor stops:

caller of StreamProcessor invokes stop()

Container completes processing (eg. bounded input) and shuts down

Container fails

Coordinator fails

When either container or coordinator stops (cleanly or due to exception), it will try to shutdown the streamprocessor. This needs to be synchronized so that only one code path gets triggered for shutdown.

If container is running,

container is shutdown cleanly and onContainerStop will trigger jobcoordinator.stop.

container fails to shutdown cleanly -> onContainerFailure will trigger jobcoordinator.stop

If container is not running, then we should simply shutdown the jobcoordinator.

@navina let's document this as part of implementation notes in the method javadoc?

fredji97 · 2017-05-01T21:38:38Z

samza-core/src/main/java/org/apache/samza/standalone/StandaloneJobCoordinator.java

+    for (String systemName: systemConfig.getSystemNames()) {
+      String systemFactoryClassName = systemConfig.getSystemFactory(systemName);
+      if (systemFactoryClassName == null) {
+        LOGGER.error(String.format("A stream uses system %s, which is missing from the configuration.", systemName));


Do we have to do both logging and exception on the same message here? I think throwing the exception and let the caller to handle the exception should be good enough.

I don't think the exception will get logged if the caller doesn't handle the exception, will it?
If exception gets logged either ways, then, I can remove the redundant log statement.

fredji97 · 2017-05-01T21:41:12Z

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

    // TODO - make a loop here with some number of attempts.
    // possibly split into two method - becomeLeader() and becomeParticipant()
    leaderElector.tryBecomeLeader(new LeaderElectorListener() {
      @Override
      public void onBecomingLeader() {
-        onBecomeLeader();
+        listenToProcessorLiveness(); // subscribe for adding new processors


the comment here does not match the method here, or it does not explain well.

Sure. I will fix it.

xinyuiscool

The refactoring looks great. Thanks for the clean up.

xinyuiscool · 2017-05-01T22:35:59Z

samza-core/src/main/java/org/apache/samza/SamzaContainerStatus.java

+  /**
+   * Indicates that the container has not been started
+   */
+  NOT_STARTED,


In ApplicationStatus we have a status "NEW", which is the same meaning as NOT_STARTED. Would you like being consistent with that?

ApplicationStatus should rely on the status of the StreamProcessor itself, and not on its individual components. Having said that, we have not defined states for StreamProcessors yet. We should, ideally, define states for StreamProcessor and that should be inspected by the ApplicationRunner for status.

I would prefer to keep SamzaContainer's initial status as NOT_STARTED because each time we rebalance, we are creating a "new" samza container instance, which is technically not started. Calling it NEW really doesn't make sense atm.

Is there any state transition details for ApplicationStatus? What it means in the standalone scenarios?

xinyuiscool · 2017-05-01T22:36:45Z

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

   * @return An instance of IJobCoordinator
   */
-  JobCoordinator getJobCoordinator(String processorId, Config config, SamzaContainerController containerController);
+  JobCoordinator getJobCoordinator(String processorId, Config config);


This change makes the interface a lot cleaner! Thx!

xinyuiscool · 2017-05-02T00:07:31Z

samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java

+      @Override
+      public void onJobModelExpired() {
+        if (container != null && container.getStatus().equals(SamzaContainerStatus.RUNNING)) {
+          container.shutdown(true);


I feel passing this flag around and then using it in the callback is kind of making the listener interface not very clean. Do you think we can set up a different listener before container.shutdown() like following:

container.setListener(new ContainerListener() {
void onStart() {//donothing}
void onStop() {// do the logic here, trigger the countdown latch}
void onfailuer() {//do the failuer logic}
);
container.shutdown();
...

Then I think we don't need this flag anymore.

I am not clear on what you proposing here.
I agree that passing around a flag is not optimal. But we have bi-directional notifications among the components here. If there is a more elegant way, we should incorporate that. Let me sync up with you in-person.

Spoke with @xinyuiscool and @prateekm offline. Summarizing it here:
Above, Xinyu was suggesting the overwriting the listener to the container depending on who invokes the shutdown. Or register more than one listener to the container and the container invokes the correct listener depending on who is requesting the shutdown.
The problem with either of the suggestion is:

we are blurring the contracts of the listener interface

container needs to be aware of the components it is interacting with. This is against the whole idea behind listener model

The main issue under discussion is the "pauseByJm" boolean flag passed to onContainerStop. This is required to identify why the container stopped or completed. If the container itself can clearly expose its state, then we wouldn't need this. Problem is that when the container "completes" processing (as in bounded or end of stream), it will automatically stop. There is no way to distinguish this state of the container from the "STOPPED" state. If we can clearly identify a "PAUSED" state and "COMPLETED" state of the SamzaContainer, we can get rid of this flag. Until such a clear definition of state becomes apparent, we have to thread the flag through the components.

Conclusion: For the sake of clarity of the code, we decided to use an "enum" to identify the source requesting the shutdown, instead of using a boolean flag.

xinyuiscool · 2017-05-02T00:12:03Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala

-    }
+  @volatile private var status = SamzaContainerStatus.NOT_STARTED
+  private var exceptionSeen: Throwable  = null
+  private var paused: Boolean = false


As mentioned above, hopefully we can remove this flag.

xinyuiscool · 2017-05-02T00:21:09Z

samza-core/src/main/java/org/apache/samza/runtime/LocalContainerRunner.java

+
+            @Override
+            public void onContainerFailed(Throwable t) {
+              log.info("Container Failed");


This needs to be handled for the yarn case. Previously container.run() will throw exception when there is a failure. Now seems the logic is changed and the exception is populated in the callback. So it seems we need to store the exception into a var and throw it after container.run(). Something like:
LocalConatinerRunner {
Throwable throwable;
...
container setContainerListener(new SamzaContainerListener() {
...
onContainerFailed(Throwable t) {
throwable = t;
}
}

container.run();

if(throwable != null) {
throw throwable
}
...

Oh you are right. I think I actually wanted to ask you about this runner and I forgot about it.

But why do we want to throw again, only to be caught by uncaughtexceptionhandler ?

I am ok with throwing it after it returns from run. It will exit the status with error code I guess. Thanks for pointing out that I changed the existing behavior :D

xinyuiscool · 2017-05-02T00:24:26Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala

+        }
+      case SamzaContainerStatus.FAILED =>
+        if (containerListener != null) {
+          containerListener.onContainerFailed(exceptionSeen)


Now we are not throw the exception directly from container.run(). We populate it into the callback. Please do a usage search of container.run() and confirm that the throwable from the callback is handled properly. I found one case needed to be fixed in the LocalContainerRunner.

I actually did a usage search and that is how I identified LocalContainerRunner. I don't see any other usage of container.run. The only callers are LocalContainerRunner, ThreadJob and StreamProcessor.
If there are custom usages of this interface through CommandBuilders (ie. using task.execute), it should be handled by those custom scripts. Let me know if you find other places of usage. I couldn't find any.

prateekm · 2017-05-02T17:41:44Z

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

@@ -28,39 +29,41 @@
 *  based on the underlying environment. In some cases, ID assignment is completely config driven, while in other
 *  cases, ID assignment may require coordination with JobCoordinators of other StreamProcessors.
 *
- *  This interface contains methods required for the StreamProcessor to interact with JobCoordinator.
+ *  This interface contains methods required for the StreamProcessor to interact with JobCoordinator. StreamProcessor


Documentation: First sentence is redundant.
"to get notified about JobModel and Coordinator state changes."

prateekm · 2017-05-02T17:43:35Z

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

@@ -55,7 +55,7 @@
  private final String processorId;

  private final ZkController zkController;
-  private final SamzaContainerController containerController;
+  private JobCoordinatorListener coordinatorListener = null;


Codestyle: Group final and non-final fields separately.

prateekm · 2017-05-02T17:44:44Z

samza-core/src/main/java/org/apache/samza/processor/JobCoordinatorListener.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.processor;


JobCoordinatorLifecycleListener. Should be in the coordinator package.

Good point.

prateekm · 2017-05-02T17:45:12Z

samza-core/src/main/java/org/apache/samza/processor/SamzaContainerListener.java

+ * Listener interface that can be registered with the {@link org.apache.samza.container.SamzaContainer} instance in
+ * order to receive notifications.
+ */
+public interface SamzaContainerListener {


SamzaContainerLifecycleListener. Should be in the container package.

Good point. Are you also suggesting we rename this SamzaContainerLifecycleListener?

Yeah, it's verbose but would prefer XLifecycleListener naming for all of these.

Just realized that the lifecycle listener that we already have internally is called SamzaContainerLifecycleListener. So, there will be a name collision if we change it now. May be we can change this later on?

prateekm · 2017-05-02T17:46:23Z

samza-core/src/main/java/org/apache/samza/processor/SamzaContainerListener.java

+public interface SamzaContainerListener {
+
+  /**
+   * <p>


Documentation: <p> are not necessary (here and everywhere else).

It's not necessary. But it looks better formatted in the javadoc. Is that not the case?

You don't need it if there's only one paragraph, javadoc will format it appropriately. If there are multiple paragraphs, you can insert
between them. E.g., http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#format

It is so odd that
is mostly used for breaklines. I removed from this class. Will adjust other docs as and when I find them.

prateekm · 2017-05-02T19:33:10Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala

-        throw ie
-    }
+  @volatile private var status = SamzaContainerStatus.NOT_STARTED
+  private var exceptionSeen: Throwable  = null


Minor: Single space before =

prateekm · 2017-05-02T19:34:21Z

samza-core/src/main/java/org/apache/samza/SamzaContainerStatus.java

+  /**
+   * Indicates that the container started the {@link org.apache.samza.container.RunLoop}
+   */
+  RUNNING,


Minor: Slightly prefer STARTED instead of RUNNING. Matches better with the previous 2 states and the lifecycle hook name (onStart). Up to you though.

Ah.. too many places to change now . Let me try to refactor in the IDE

prateekm · 2017-05-02T19:36:05Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala

-        throw e
-    } finally {
+        if (status.equals(SamzaContainerStatus.RUNNING)) {
+          error("Caught exception/error in process loop.", e)


Minor: "in run loop."

Makes sense.

prateekm · 2017-05-02T19:37:58Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala

    }
  }

-  def shutdown() = {
+  /**
+   * Triggers shutdown of the run loop


Same as the comment for StreamProcessor. Maybe:

Asynchronously shuts down this SamzaContainer. Implementation:

prateekm · 2017-05-02T20:43:31Z

samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala

@@ -54,18 +52,31 @@ class ThreadJobFactory extends StreamJobFactory with Logging {
      case _ => None
    }

+    val containerListener = new SamzaContainerListener {
+      override def onContainerFailed(t: Throwable): Unit = {
+        throw t


Will the thrown exception be handled correctly or result in an uncaught exception and jvm quitting?

I think the behavior should be same as before. ThreadJob does have a try-catch and it re-throws the Throwable. Not sure if the JobRunner will handle it though. There isn't any uncaught exception handler in thread job. So, may be ApplicationRunner should deal with it??

Can't ThreadJob be run w/o the ApplicationRunner (i.e. with the low-level APIs)?

Iiuc, ApplicationRunner is the user-facing api, even for thread jobs.

@xinyuiscool can you please clarify ?

Looks good to me. Log it please.

navina · 2017-05-03T07:08:37Z

@prateekm @xinyuiscool I have addressed all the comments. Only exception is the one involving the boolean flag. Replacing it with the enum makes it easy to read. But I don't think we have clear values for the enum - except for JobCoordinator.
On second thought, would it be better to keep a separate method called "pause" in SamzaContainer and that is used to set the paused flag. The benefit of this approach is:

It will be more readable and we won't semantically overload the shutdown method
In future, when we refactor container to have a well-defined paused status, it should be relatively simple to transition over.

What do you think?

xinyuiscool · 2017-05-03T16:31:17Z

+1. I like adding the pause() much better than passing the flag or enum around. We can have some comments saying it's actually stopping the container right now, but the code is much cleaner.

prateekm · 2017-05-03T16:53:14Z

My first preference would be to try to do the bookkeeping in StreamProcessor for whether JC or SP requested shutdown, if possible. If we use the enum instead, how about the following values: JobCoordinator, StreamProcessor, SamzaContainer (EOS), LocalContainerRunner (for now) I'd prefer either over adding pause before it's an actual SamzaContainer state.

…

On Wed, May 3, 2017 at 9:31 AM, xinyuiscool ***@***.***> wrote: +1. I like adding the pause() much better than passing the flag or enum around. We can have some comments saying it's actually stopping the container right now, but the code is much cleaner. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#148 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABCRo1ZvwNDunOuLm0JU7K7Ip9zmAWmoks5r2KvXgaJpZM4NLJ3C> .

navina · 2017-05-03T17:48:40Z

My first preference would be to try to do the bookkeeping in
StreamProcessor for whether JC or SP requested shutdown, if possible.

That would be the most ideal case. But it is not as straightforward as we think. We have to synchronize access and ordering of calls between jobcoordinator and container. So, I would like to table this approach for now.

If we use the enum instead, how about the following values:
JobCoordinator, StreamProcessor, SamzaContainer (EOS), LocalContainerRunner
(for now)

I can add values for the enum. But in reality, the only enum for which you will have a different behavior is when the value is JobCoordinator. The rest of them are unusable atm or don't have any functional value. I would prefer not to add enums just for the sake of it. It only feels like more tech-debt for the future and unlikely, that the debt is resolved in the "near-future".

I'd prefer either over adding pause before it's an actual SamzaContainer state.

The only problem we are actually facing right now is how to make the code more readable. I believe pause solves the problem reasonably.

Functionally, there is not a lot we can change. I tried out the Enum approach and didn't like it because it was ignoring all values other than JobCoordinator.

navina · 2017-05-03T17:53:43Z

@prateekm another point that I missed above. It is actually the Streamprocessor that decides that when the jobmodel changes, it will stop the container. So, should the enum used be JobCoordinator or StreamProcessor. For all shutdown request from the listener, it should theoretically be StreamProcessor because JobCoordinator doesn't care about the state of the container.
This aspect was confusing when using the enum approach.

prateekm

Thanks, LGTM!

prateekm · 2017-05-03T20:45:07Z

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

+
+
+  /**
+   * <p>


Minor: Remove <p> in this class too.

xinyuiscool · 2017-05-03T21:21:35Z

samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala

@@ -54,18 +52,31 @@ class ThreadJobFactory extends StreamJobFactory with Logging {
      case _ => None
    }

+    val containerListener = new SamzaContainerListener {
+      override def onContainerFailed(t: Throwable): Unit = {
+        throw t


Looks good to me. Log it please.

…ller

…tener, SamzaContainerListener

…ener for ThreadJob

…lerImpl

navina mentioned this pull request May 1, 2017

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

Closed

fredji97 reviewed May 1, 2017

View reviewed changes

xinyuiscool reviewed May 2, 2017

View reviewed changes

prateekm reviewed May 2, 2017

View reviewed changes

navina force-pushed the SAMZA-1212 branch from 6f0715c to aab36a9 Compare May 3, 2017 07:03

prateekm approved these changes May 3, 2017

View reviewed changes

xinyuiscool approved these changes May 3, 2017

View reviewed changes

navina force-pushed the SAMZA-1212 branch from 5a9155c to 0d377e2 Compare May 3, 2017 21:41

navina added 16 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

navina added 18 commits May 3, 2017 15:07

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

Adding a pause interface to SamzaContainer

9baa071

Another documentation fix

a5d277b

Fixing javadoc formatting

4cf6020

Adding log in ThreadJob's onContainerFailed

fc0fdbe

navina force-pushed the SAMZA-1212 branch from 0d377e2 to fc0fdbe Compare May 3, 2017 22:08

asfgit closed this in 475b465 May 3, 2017

navina mentioned this pull request May 3, 2017

SAMZA-1084 - User thread does not see errors from the processor thread #54

Closed

SAMZA-1212 - Refactor interaction between StreamProcessor, JobCoordinator and SamzaContainer #148

SAMZA-1212 - Refactor interaction between StreamProcessor, JobCoordinator and SamzaContainer #148

Conversation

navina commented Apr 28, 2017

navina commented Apr 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

navina commented May 3, 2017

xinyuiscool commented May 3, 2017

prateekm commented May 3, 2017 via email

navina commented May 3, 2017

navina commented May 3, 2017

prateekm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment