You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by "mynameborat (via GitHub)" <gi...@apache.org> on 2023/08/16 07:52:02 UTC
[GitHub] [samza] mynameborat opened a new pull request, #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages
mynameborat opened a new pull request, #1681:
URL: https://github.com/apache/samza/pull/1681
**Description**:
Currently, watermark is implemented as a special message within Samza. However, in terms of processing semantics, it shares similar behavior to normal messages processed by the task. i.e., `task.callback.timeout.ms`, a configuration to tune the time until which runloop waits for a message to be processed applies to both watermark and normal messages.
However, this tie up constrains watermark processing logic to be bounded by the processing messages time bound. For Beam on Samza, we use watermark as a trigger to execute event timers which can take a long time depending on the number of timers accumulated. Especially, when the application is down, the timers accumulated could be too large and users will have to tune this configuration which will also impact fault tolerance behavior in case of failures/delays during processing messages.
**Changes**:
- Introduce callback timeout configuration specific to watermark
- Update configuration documentation
- Consolidate overload methods for `TaskCallbackManager`
- Always use watermark specific timeout even when run loop is in draining mode
**Tests**:
- Added unit test for `TestRunLoop`
- Added unit tests for `RunLoopConfig`
**API Changes**:
- Internal change to constructor
**Upgrade Instructions**: None
**Usage Instructions**:
- Users can configure the timeout for watermark messages using `task.callback.watermark.timeout.ms`
- Refer to the configuration documentation for more details and defaults.
**Note**: Pull request is stacked on #1680 . Please review after 1st commit.
--
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
[GitHub] [samza] dxichen commented on a diff in pull request #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages
Posted by "dxichen (via GitHub)" <gi...@apache.org>.
dxichen commented on code in PR #1681:
URL: https://github.com/apache/samza/pull/1681#discussion_r1305926076
##########
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:
Do we ever expect to set the default for drain higher than this watermark timeout? If that is the case it will impact existing drain behavior causing it to default to task.callback.ms on watermarks. We could consider taking the `Max(watermark timeout, drain timeout)` when they are both enabled
if watermark should be higher than drain when both conditions apply, lets state that assumption
--
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
[GitHub] [samza] mynameborat commented on a diff in pull request #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages
Posted by "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
[GitHub] [samza] mynameborat merged pull request #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat merged PR #1681:
URL: https://github.com/apache/samza/pull/1681
--
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
[GitHub] [samza] mynameborat commented on pull request #1681: SAMZA-2791: Introduce callback timeout specific to watermark messages
Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on PR #1681:
URL: https://github.com/apache/samza/pull/1681#issuecomment-1698112234
@ajothomas @dxichen please take a look
--
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