barakbaron12 commented on PR #27334: URL: https://github.com/apache/flink/pull/27334#issuecomment-4311181664
Quick update to both @davidradl and @rmetzger: @davidradl — thanks for the review! I've addressed the feedback in commit e1947768f57: 1. **Medium – invalid parallelism values**: Added validation. Non-numeric values and non-positive integers now throw `IllegalArgumentException` with a clear message identifying the vertex and the invalid value. Added tests for both cases. 2. **Low – inconsistent placement in `AdaptiveBatchSchedulerFactory`**: Refactored to use a new overload `ParallelismOverrideUtil.applyParallelismOverridesIfApplicable(ExecutionPlan, ...)` that no-ops for `StreamGraph` inputs. This lets `AdaptiveBatchSchedulerFactory` call the utility at the same structural position as the other two scheduler factories, outside the type-check branch. That said, the new approach isn't perfect either — the overload hides the branching logic behind a method name, which is arguably still "inconsistent" in a different way. The `IfApplicable` suffix tries to hint that it may no-op, but readers still need to read the javadoc to understand why. If you have a better pattern in mind I'd really appreciate your thoughts! 3. **Low – documentation**: Expanded the class javadoc to cover what the feature does, the two config sources and their precedence, where the utility is called from, and the adaptive-batch `StreamGraph` path handled by `DefaultAdaptiveExecutionHandler`. 4. **Low – reference FLINK-38770 in the inline comment**: I'd gently push back on this one. JIRA refs don't seem to be common in inline comments in the codebase — they tend to live in commit messages, PR descriptions, and the public issue tracker. My concern is that references in source code can get stale when issues get moved/renamed. Happy to add it if you feel strongly. @rmetzger — good question. The honest answer is that there were no tests covering parallelism overrides in Application Mode before this PR. All existing tests submitted `JobGraph`s directly (Session Mode), which goes through the code path where overrides were applied in the Dispatcher. Application Mode submits a `StreamGraph` and the override call was never hit for that path — so the feature silently did nothing. The bug surfaced via the Flink Kubernetes Operator autoscaler, which uses this feature to dynamically adjust parallelism on a running application (as @voxeljorge mentioned above, they backported this fix to stay on 2.0.1 in production). This PR closes that gap. We now have coverage for all three schedulers in Application Mode: - `ParallelismOverridesITCase` — Default scheduler, including partial overrides and cluster-vs-job precedence - `AdaptiveSchedulerITCase#testParallelismOverridesInApplicationMode` - `AdaptiveBatchSchedulerITCase#testParallelismOverridesInApplicationMode` I'm happy to add an end-to-end test if that would be useful — any suggestion on where it would fit best in `flink-end-to-end-tests`? -- 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]
