Introduce initialization exception handler in KCL V2 #369

jiaxul · 2018-08-15T22:44:12Z

Issue #, if available:

Introduce initialization exception handler in KCL V2.

KCL scheduler does not throw any exceptions that occurs during processing a stream. It just logs it as an error.

Description of changes:
add a CoordinatorExceptionHandler interface
add CoordinatorExceptionHandler as a parameter in CoordinatorConfig
add a NoOpCoordinatorExceptionHandler to pass in as default so it does not change the behavior of the Scheduler class
The client applications can implement the CoordinatorExceptionHandler and pass in CoordinatorConfig so that they can specify how they want to handle the exceptions during processing the streams.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sahilpalvia

Some initial comments

sahilpalvia · 2018-08-15T23:43:25Z

...lient/src/main/java/software/amazon/kinesis/coordinator/NoOpCoordinatorExceptionHandler.java

+
+package software.amazon.kinesis.coordinator;
+
+public class NoOpCoordinatorExceptionHandler implements CoordinatorExceptionHandler {


A better name would be PassThroughCoordinatorExceptionHandler.

sure, will change

sahilpalvia · 2018-08-15T23:43:37Z

...is-client/src/main/java/software/amazon/kinesis/coordinator/CoordinatorExceptionHandler.java

+
+package software.amazon.kinesis.coordinator;
+
+/**


Could you add documentation

sahilpalvia · 2018-08-15T23:43:58Z

...is-client/src/main/java/software/amazon/kinesis/coordinator/CoordinatorExceptionHandler.java

+ *
+ */
+public interface CoordinatorExceptionHandler {
+    void propogate(Exception e);


Same as above, some documentation.

sahilpalvia · 2018-08-15T23:44:40Z

...lient/src/main/java/software/amazon/kinesis/coordinator/NoOpCoordinatorExceptionHandler.java

+    /**
+     * Empty constructor for NoOp Exception Handler
+     */
+    public NoOpCoordinatorExceptionHandler() {


You could use @NoArgumentConstructor from lombok

Sure. I was trying to be consistent with, for example, NoOpWorkerStateChangeListener.java and NoOpShardPrioritization.java. They are having empty constructors

sahilpalvia · 2018-08-15T23:50:41Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/coordinator/Scheduler.java

        }

        while (!shouldShutdown()) {
+            exceptionToThrow = null;


Need not set this. If exception is caught from initialize, shutdown is invoked and shouldShutdown will return true if the Scheduler can successfully shutdown. It can enter to make sure that ShardConsumers have been shutdown successfully.

Will delete

…ass names

pfifer · 2018-08-17T19:23:49Z

...-client/src/main/java/software/amazon/kinesis/coordinator/NoOpWorkerStateChangeListener.java

@@ -27,4 +27,9 @@ public NoOpWorkerStateChangeListener() {
 	public void onWorkerStateChange(WorkerState newState) {

 	}
+
+	@Override
+	public void onInitializationError(Exception e) {


This should be a default on the interface.
Would prefer the name to be onInitializationException

pfifer · 2018-08-17T19:25:14Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/coordinator/Scheduler.java

@@ -193,12 +193,14 @@ public void run() {
            return;
        }

+        Exception exceptionToThrow = null;


This doesn't feel like the right place for this. Should we instead put onInitializationException at the point that the exception was thrown?
This just reports the last exception, which means a cascaded failure may be misreported.

pfifer · 2018-08-17T19:26:30Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/coordinator/Scheduler.java

@@ -207,6 +209,10 @@ public void run() {

        finalShutdown();
        log.info("Worker loop is complete. Exiting from worker.");
+
+        if (exceptionToThrow != null) {


I don't feel this is the correct location to report the exception. We should report as near as possible to where the exception originally occurred. This would be important if the user was also attempting to create a stack trace for the original call site.

pfifer · 2018-08-17T19:27:33Z

...esis-client/src/main/java/software/amazon/kinesis/coordinator/WorkerStateChangeListener.java

@@ -27,4 +26,6 @@
 	}

 	void onWorkerStateChange(WorkerState newState);
+
+	void onInitializationError(Exception e);


This is a breaking change, and must be defaulted.

I also want to think more about how state reporting and actions happen. Finally this should accept a Throwable vs an Exception.

…erStateChangeListener interface and call it when scheduler determines an initialization is failed

sahilpalvia

Looking good, a few more comments to be addressed.

sahilpalvia · 2018-08-29T21:57:06Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/coordinator/Scheduler.java

@@ -199,6 +199,7 @@ public void run() {
        } catch (RuntimeException e) {
            log.error("Unable to initialize after {} attempts. Shutting down.", lifecycleConfig.maxInitializationAttempts(), e);
            shutdown();
+            workerStateChangeListener.onInitializationFailure(e);


It would be better to invoke the handler before calling shutdown. The handler is called as a reaction of the exception. It needs to be invoked as soon as the exception is thrown before performing any other action.

sahilpalvia · 2018-08-29T21:57:48Z

...esis-client/src/main/java/software/amazon/kinesis/coordinator/WorkerStateChangeListener.java

@@ -27,4 +27,7 @@
 	}

 	void onWorkerStateChange(WorkerState newState);
+
+	default void onInitializationFailure(Throwable e) {


Can you rename the method to something like allInitializationAttemptsFailed(Throwable t), indicating that the handler is invoked after all the initialization attempt have failed.

sahilpalvia · 2018-08-29T21:59:28Z

amazon-kinesis-client/src/main/java/software/amazon/kinesis/coordinator/Scheduler.java

@@ -207,6 +208,7 @@ public void run() {

        finalShutdown();
        log.info("Worker loop is complete. Exiting from worker.");
+


Please avoid whitespace changes

sahilpalvia · 2018-08-29T21:59:32Z

...-client/src/main/java/software/amazon/kinesis/coordinator/NoOpWorkerStateChangeListener.java

@@ -27,4 +27,5 @@ public NoOpWorkerStateChangeListener() {
 	public void onWorkerStateChange(WorkerState newState) {

 	}
+


Please avoid whitespace changes

Introduce initialization exception handler in KCL V2

5205d25

sahilpalvia suggested changes Aug 15, 2018

View reviewed changes

sahilpalvia reviewed Aug 15, 2018

View reviewed changes

jiaxul added 3 commits August 16, 2018 14:24

added javadocs for CoordinatorExceptionHandler and modified method/cl…

73d3320

…ass names

Move exception handling to WorkerStateChangeListener

7d1d091

Remove WorkerState InitializationError

31f2ce6

pfifer suggested changes Aug 17, 2018

View reviewed changes

make initialization failure handler function a default method in Work…

c8404ef

…erStateChangeListener interface and call it when scheduler determines an initialization is failed

sahilpalvia added the v2.x Issues related to the 2.x version label Aug 20, 2018

change Exception to Throwable

fbecf73

pfifer approved these changes Aug 29, 2018

View reviewed changes

sahilpalvia suggested changes Aug 29, 2018

View reviewed changes

jiaxul and others added 4 commits August 30, 2018 10:55

change method name and invoke position

5f841a9

merge conflicts

8f9f8b7

change method name

d52873b

Merge branch 'master' into development

20209c2

pfifer merged commit ea49eef into awslabs:master Aug 31, 2018

pfifer added this to the v2.0.2 milestone Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce initialization exception handler in KCL V2 #369

Introduce initialization exception handler in KCL V2 #369

jiaxul commented Aug 15, 2018

sahilpalvia left a comment

sahilpalvia Aug 15, 2018

jiaxul Aug 16, 2018

sahilpalvia Aug 15, 2018

jiaxul Aug 16, 2018

sahilpalvia Aug 15, 2018

jiaxul Aug 16, 2018

sahilpalvia Aug 15, 2018

jiaxul Aug 16, 2018

sahilpalvia Aug 15, 2018 •

edited

jiaxul Aug 16, 2018

pfifer Aug 17, 2018

pfifer Aug 17, 2018

pfifer Aug 17, 2018

pfifer Aug 17, 2018

sahilpalvia left a comment

sahilpalvia Aug 29, 2018

sahilpalvia Aug 29, 2018

sahilpalvia Aug 29, 2018

sahilpalvia Aug 29, 2018


		package software.amazon.kinesis.coordinator;

		public class NoOpCoordinatorExceptionHandler implements CoordinatorExceptionHandler {

		@@ -207,6 +208,7 @@ public void run() {

		finalShutdown();
		log.info("Worker loop is complete. Exiting from worker.");

		@@ -27,4 +27,5 @@ public NoOpWorkerStateChangeListener() {
		public void onWorkerStateChange(WorkerState newState) {

		}

Introduce initialization exception handler in KCL V2 #369

Introduce initialization exception handler in KCL V2 #369

Conversation

jiaxul commented Aug 15, 2018

sahilpalvia 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

sahilpalvia Aug 15, 2018 • 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

sahilpalvia 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

sahilpalvia Aug 15, 2018 •

edited