Repository navigation
[runtime][java] Complete batch workflows before key switches - #1172
Conversation
|
Thanks for the contribution! I’m currently focused on preparing for the 0.4 release, so it may take me a while to review this PR. Thanks for your patience, and other reviewers are welcome to take a look in the meantime. |
sangkyoonnam
left a comment
There was a problem hiding this comment.
After rebasing onto main, the batch drain needs to move outside the parallel engine's lock. Where it sits now, it fails the task under the batch backend whenever that engine is picked. #1147 merged the day after this PR opened, so the interaction didn't exist at your base. The rebase also hits one import conflict in FlinkIntegrationTest.
What I ran, Corretto 17.0.14:
- PR head:
CompileUtilsTestpasses 12/12. With onlyActionExecutionOperator.javareverted to its parent, the three new tests fail with[10]instead of[2, 4, 6, 8, 10]and[60]instead of[20, 40, 60]. - PR head merged with
upstream/main(d6e2ecc9): the same three tests error withIllegalStateException: ParallelExecutionLock is non-reentrant; the owner cannot acquire it again.JDK 21 gives the same result in unit tests. I think that's because surefire runs fromtarget/classes, where theMETA-INF/versions/21continuation classes aren't picked up, so these plans still get the parallel engine. I haven't tested the packaged JDK 21 continuation path.
This comes from the drain meeting #1147's lock wrapper. For these Java-only test plans, the parallel engine is selected by default when continuation support isn't available. On that engine processElement runs processElementInternal inside runWithParallelExecutionLock. Both waitForCurrentInputInBatchMode() calls are in processElementInternal, and waitInFlightEventsFinished() now takes the lock itself through acquireByMain(). Making only the lock reentrant wouldn't fix this. Yielding while holding it would block the workers, which need it to run actions.
On the rebased code, this change fixes it in my runs:
@@ -468,6 +468,7 @@
} else {
processElementInternal(record);
}
+ waitForCurrentInputInBatchMode();
}
private void processElementInternal(StreamRecord<IN> record) throws Exception {
@@ -501,4 +502,3 @@
enqueuePendingInputEvent(inputEvent);
- waitForCurrentInputInBatchMode();
return;
}
@@ -512,3 +512,2 @@
processInputEvent(key, inputEvent);
- waitForCurrentInputInBatchMode();
}With that on the merged branch, ActionExecutionOperator*Test plus CompileUtilsTest pass, 89 tests, 0 failures. I also ran the synchronous five-key batch case with parallel-execution.enabled=false as a local-only test. It passes before and after the move. I haven't run the e2e FlinkIntegrationTest or the Python path.
| // Otherwise, the new event is processed immediately. Its failures are attributed to the | ||
| // input run created by processInputEvent. | ||
| processInputEvent(key, inputEvent); | ||
| waitForCurrentInputInBatchMode(); |
There was a problem hiding this comment.
In the rebased code, this call and the one at L311 run while processElement holds ParallelExecutionLock. waitInFlightEventsFinished() then tries to acquire it again. I'd remove both and add one call after the if/else in processElement, as in the summary. The drain still finishes before the next record can switch keys.
Could you add a serial variant of the five-key batch test with AgentExecutionOptions.PARALLEL_EXECUTION_ENABLED=false in the plan config? After rebasing, I'd also pin one batch test to the parallel engine: set the option to true and add assumeFalse(ContinuationActionExecutor.isContinuationSupported()), like the parallel tests in ActionExecutionOperatorTest. The current batch tests rely on the default engine selection.
There was a problem hiding this comment.
Optional cleanup: I'd remove runtime/API.md and runtime/Method.md. Nothing in the repository references them, and they sit outside docs/content, so the docs build doesn't publish them. The Javadoc on waitForCurrentInputInBatchMode already states the contract. If no user-facing doc change is planned, doc-not-needed fits better than doc-included. A note that BATCH now works would belong under docs/content.
|
Sorry for the delayed review—I’ve been focused on the 0.4 release work. Thanks for your patience and for working on this! We explored the alternative of setting However, both settings apply job-wide and default to It would be helpful to document this tradeoff and retain regression coverage for the disabled-backend configuration alongside the new default-backend tests. WDYT? |
c5d5a55 to
8ba0573
Compare
Drain each batch input after releasing the parallel execution lock and remove the initialization guard that rejected the batch state backend. Cover serial and parallel execution, repeated-key memory, cross-key concurrency with the regular backend, and Python async workflows. Document the job-wide backend and sorted-input configuration tradeoffs. Follow-up to apache#1172; fixes apache#939. Validation: 117 Java tests and 6 Python integration tests passed with Flink 2.3 and JDK 11. Spotless, Ruff, and git diff --check passed. Generated-by: Codex 0.153.4 (GPT-6) Co-authored-by: Codex <codex@openai.com>
|
Thanks @wenjin272 — 8ba0573 covers the points from the review:
|
Drain action continuations before returning from processElement when Flink uses its batch keyed-state backend. This preserves per-key state across the full workflow while leaving streaming cross-key concurrency unchanged. Replace the temporary BATCH rejection with synchronous, asynchronous, AUTOMATIC-mode, and multi-version end-to-end coverage. Generated-by: TraeCode (GPT-5)
Drain each batch input after releasing the parallel execution lock and remove the initialization guard that rejected the batch state backend. Cover serial and parallel execution, repeated-key memory, cross-key concurrency with the regular backend, and Python async workflows. Document the job-wide backend and sorted-input configuration tradeoffs. Follow-up to apache#1172; fixes apache#939. Validation: 117 Java tests and 6 Python integration tests passed with Flink 2.3 and JDK 11. Spotless, Ruff, and git diff --check passed. Generated-by: Codex 0.153.4 (GPT-6) Co-authored-by: Codex <codex@openai.com>
8ba0573 to
47dcdbd
Compare
|
Thanks for confirming, @wellkilo, and thanks @sangkyoonnam for the detailed review! I’ve also rebased this PR onto the latest main, which includes #1371’s fix for the unrelated CI failure in The focused tests passed locally after the rebase: 118 passed and 2 skipped on JDK 11. Let’s see how the updated CI run goes. |
Linked issue: #939
Closes #939
Purpose of change
Flink Agents workflows now complete every keyed input when Flink uses its batch keyed-state backend, instead of rejecting explicit batch mode or silently retaining only the last key's output. Bounded
AUTOMATICjobs receive the same behavior when Flink selects that backend.Runtime flow
Both Java and Python entry points still converge on
CompileUtils.connectToAgent. The previous graph-construction rejection is removed. DuringActionExecutionOperator.open, the operator records whether Flink created aBatchExecutionKeyedStateBackend.processElementcontinues to enqueue action work through the mailbox; before it returns under that backend, it yields until the current input's complete action chain has removed its processing key. General-purpose keyed-state backends skip this drain and preserve cross-key concurrency.Key decisions
BATCHand boundedAUTOMATICjobs.waitInFlightEventsFinishedpath instead of introducing a second synchronous action executor or moving all keyed runtime state into operator-owned maps.Behavioral Semantics
Interaction decisions
processElementreturnsBehavioral contracts
AUTOMATICmode behaves correctly when Flink resolves bounded input to batch execution.Failure behavior
Mailbox continuation failures still propagate through the operator's existing action-task failure path and fail the task. The batch drain does not retry, fall back, or absorb failures. No new configuration or external-service failure path is introduced.
Tests
CompileUtilsTest.processesEveryKeyWithDefaultBatchStateBackend;FlinkIntegrationTest.testBatchExecutionWithMultipleKeysCompileUtilsTest.processesAsyncActionsWithDefaultBatchStateBackendCompileUtilsTest.processesEveryKeyWhenAutomaticModeSelectsBatchExecutionActionExecutionOperatorTest.testDifferentKeyDataCanRunConcurrently;testExecuteAsyncWithMultipleKeysVerification:
FlinkIntegrationTest.testBatchExecutionWithMultipleKeyspassed on Flink 1.20.5 and 2.3.0 locally; CI runs the same test on every supported Flink line from 1.20 through 2.3.Not verified locally: the E2E test was not individually executed against Flink 2.0, 2.1, and 2.2; their dist builds succeeded, and the repository's existing Java IT matrix runs the test for each version.
Implementation invariants and supporting evidence
[10]instead of[2, 4, 6, 8, 10].BatchExecutionKeyedStateBackendis present under the same fully qualified name in the supported Flink 1.20.5, 2.0.2, 2.1.3, 2.2.1, and 2.3.0 artifacts.API
No public signature, JSON, event, or IDL changes. Existing callers using explicit batch mode no longer receive the temporary configuration-time
IllegalStateException; the previously unsupported configuration now executes correctly. Java and Python bridge calls share the same runtime operator behavior.Documentation
doc-neededdoc-not-neededdoc-includedThe runtime method and API contracts are recorded in
runtime/Method.mdandruntime/API.md.Was this patch authored or co-authored using generative AI tooling?
Generated-by: TraeCode (GPT-5)