vsantwana commented on PR #1082: URL: https://github.com/apache/flink-kubernetes-operator/pull/1082#issuecomment-4766649005
@gyfora thanks for the review. Summary of the changes in this update: **Addressing your comments** - **Consolidated the memory/resource utility methods** into a single unit-testable class `ResourceConfigUtils` (`parseResourceMemoryString`, `hasResourceRequirements`, `getRequestOrLimit`, `computeLimitFactor`, `isMemoryDefined`). It's now the single source used by `FlinkConfigBuilder`, `KubernetesScalingRealizer`, and `DefaultValidator`, with dedicated unit tests (incl. mixed-unit cases). This removed the scattered static helpers and a good chunk of duplication. - **Updated the documentation-embedded examples** (`overview.md`, `pod-template.md`, `job-management.md`) to the new `resources` (ResourceRequirements) format. **Additional cleanup / hardening while in here** - **Removed the JM/TM duplication** in `FlinkConfigBuilder`: the `resources`-vs-deprecated-`resource` dispatch (previously copy-pasted across `applyPodTemplate` and `applyJobManagerSpec`/`applyTaskManagerSpec`) is now a single helper each. - **Validation hardening** in `DefaultValidator` for the new `resources` path: reject non-positive cpu/memory and `requests > limits` (mixed-unit aware), and a warning is logged when both `resource` and `resources` are set (the deprecated field is ignored in favor of `resources`). - **Minor**: fixed a doc-generator bug where the `@deprecated` Javadoc tag ran into the preceding description in the generated `reference.md` (`pods.@deprecated` -> `pods. @deprecated`); used `CrdConstants` for resource keys in test fixtures. - **Tests**: end-to-end pod-template test for the new `resources` path (JM+TM ephemeral-storage wiring) plus validator tests for zero / `requests > limits`. **One open question for you** The old `resource` and new `resources` paths are *not* fully symmetric in validation. After this change the new path is stricter (cpu validated + positivity + requests<=limits), whereas the deprecated `validateResources` never validated cpu at all. More importantly, **both** paths share one gap: when memory is defined via `resource.memory` *or* `resources.requests.memory`, the Flink process-spec memory check (`validateJmMemoryConfig`/`validateTmMemoryConfig`) is skipped, so a too-small memory value passes validation and only fails at deploy. This is pre-existing behavior (not introduced here), so I left it as-is. Do you want me to (a) make the deprecated `validateResources` reuse the same quantity checks for consistency, and/or (b) actually validate the configured memory against the process spec on both paths? Happy to do either in this PR or a follow-up. -- 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]
