github-actions[bot] commented on code in PR #65618:
URL: https://github.com/apache/doris/pull/65618#discussion_r3584189391
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -1234,7 +1234,7 @@ public class Config extends ConfigBase {
* Minimum batch interval for adaptive routine load tasks when not at EOF.
*/
@ConfField(mutable = true, masterOnly = true)
- public static int routine_load_adaptive_min_batch_interval_sec = 360;
+ public static int routine_load_adaptive_min_batch_interval_sec = 120;
Review Comment:
This default is shared by Kafka and Kinesis, but the Kinesis path still does
not update the task timeout before the transaction is opened. The scheduler
calls `updateAdaptiveTimeout()` before `beginTxn()`, and `beginTxn()` persists
the current `timeoutMs` into the transaction; Kafka overrides that hook, but
`KinesisTaskInfo` only applies `routine_load_adaptive_min_batch_interval_sec`
later in `adaptiveBatchParam()` while building the thrift task. With the new
default, a normal 60s Kinesis job can therefore start a transaction with a
600000 ms timeout while the task/plan is sent with the 1200s adaptive timeout.
Please apply the same pre-begin adaptive timeout update to Kinesis (ideally
with the same min-timeout clamp helper as Kafka) before changing this shared
default.
##########
regression-test/suites/load_p0/routine_load/test_routine_load_timeout_value.groovy:
##########
@@ -215,7 +215,7 @@ suite("test_routine_load_timeout_value","nonConcurrent") {
sql "stop routine load for ${jobName}"
sql "DROP TABLE IF EXISTS ${tableName} FORCE"
// Restore routine_load_adaptive_min_batch_interval_sec to default
value
- sql "ADMIN SET FRONTEND CONFIG
('routine_load_adaptive_min_batch_interval_sec' = '360')"
+ sql "ADMIN SET FRONTEND CONFIG
('routine_load_adaptive_min_batch_interval_sec' = '120')"
Review Comment:
This restore is still after two fallible cleanup statements. If anything
fails after line 83 sets `routine_load_adaptive_min_batch_interval_sec` to `5`
but before the routine-load job exists, `stop routine load` throws
(`checkPrivAndGetJob` reports no operable job) and this line is skipped,
leaving the FE process with the non-default value for later `nonConcurrent`
suites. Please make the config restore unconditional, for example by moving it
before best-effort `try_sql` cleanup or putting it in an outer `finally` that
cleanup errors cannot bypass.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
