[GitHub] [samza] mynameborat commented on a diff in pull request #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages

["mynameborat (via GitHub)" <gi...@apache.org>]

mynameborat commented on code in PR #1681:
URL: https://github.com/apache/samza/pull/1681#discussion_r1307781985


##########
samza-core/src/main/java/org/apache/samza/container/RunLoop.java:
##########
@@ -629,9 +640,22 @@ public TaskCallback createCallback() {
           containerMetrics.processes().inc();
           // report 1 whenever the contaienr is running. Can be used to calculate the number of containers not running
           containerMetrics.containerRunning().set(1L);
-          return isDraining && (envelope.isDrain() || envelope.isWatermark())
-              ? callbackManager.createCallbackForDrain(task.taskName(), envelope, coordinator, drainCallbackTimeoutMs)
-              : callbackManager.createCallback(task.taskName(), envelope, coordinator);
+
+          /*
+           * Timeout used in the task callback. The value is determined based on the following logic
+           * 1. If run loop is in draining mode and the envelope is drain, use drainCallbackTimeoutMs
+           * 2. If the envelope is watermark, use watermarkCallbackTimeoutMs regardless of the modes

Review Comment:
   Good callout. I will update that note in the config documentation. I'd keep using the watermark timeout as is and not introduce another branching logic of max and have users/tools configure things appropriately.



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