wenjin272 commented on code in PR #961: URL: https://github.com/apache/flink-agents/pull/961#discussion_r3763463142
########## review-guides/api-contract.md: ########## @@ -0,0 +1,72 @@ +# Review Guide: api/ Contract + +Load this guide when a PR changes a public API surface: a signature or type in +`api/`, a new resource implementation, a config option, a YAML-visible name, or +anything a user's agent code calls. It narrows the full passes in +`code_review.md` to the ones that matter most for this area; the general passes +still apply. + +## Focused checklist + +- When a PR adds a public resource implementation, check that its short YAML + alias landed in both alias tables and the doc table. Nothing fails when all + three are skipped: each loader passes an unrecognized name through unchanged, + so the class stays reachable by fully-qualified name and no test notices the + omission. +- Regenerate the cross-language snapshots on both sides in the same change when + a field on a built-in event or on the agent plan is added, renamed, or + retyped. Each language pins its own serialization against its own committed + file, so refreshing one side leaves the other side's stability test failing. A + field added in only one language is caught by nothing, because the payload is + a free-form attribute map on the read side. +- Check that a new public config option landed on both languages' sides. A + Java-only option passes every fast CI job: the bidirectional parity check runs + only in the slow cross-language lane, and the in-tree guard is a hardcoded + count on the Python side. +- Treat the public base classes users extend as source-compatibility boundaries. + A new abstract method breaks every implementation, including ones outside this + repo, while a defaulted overload plus a capability probe does not. The Python + guard that catches a mis-declared override only sees connections whose module + is imported by hand at the top of the test. +- Check that a removal is complete rather than asking whether to deprecate. + There is no deprecation mechanism in this repo, so an API is either kept or + deleted outright. +- Name the docs the change invalidates. Config keys, YAML aliases, and whole + code samples are restated by hand across the doc site, and nothing in pull + request CI builds or checks them, so a doc that contradicts the code ships + green. Java code under `examples/` is in the Maven reactor and breaks loudly, + but nothing imports or runs the Python examples. + +## Validation + +Run both lanes. A change verified in one language only is untested in the other. + +- Java, from the repo root: `mvn --batch-mode test -pl api`. +- Python, from `python/`: `uv sync --extra test`, then `uv pip install + apache-flink`, then `uv run --no-sync pytest flink_agents/api Review Comment: I would prefer deriving the version from the root POM rather than hard-coding `2.3.0`. The Java validation command already uses its `flink.version`, so sharing that source of truth keeps both lanes aligned and prevents the guide from becoming stale after a future version bump. You also read `tools/ut.sh` correctly. It defaults to Flink 2.2 when `-f` is omitted, despite claiming to run all versions. Additionally, its non-E2E Java path ignores `flink_versions` and uses the root POM’s Flink 2.3.0, while the default Python path installs Flink 2.2.x. Multi-version coverage comes from the CI integration-test matrices explicitly passing `-e -f`, not from the script’s default behavior. This is worth tracking separately and further supports using the root POM, rather than `tools/ut.sh`, as the version source here. -- 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]
