weiqingy commented on code in PR #961: URL: https://github.com/apache/flink-agents/pull/961#discussion_r3764070605
########## 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: Done, in both guides: ``` uv pip install "apache-flink==$(mvn -q -N --batch-mode -f ../pom.xml help:evaluate -Dexpression=flink.version -DforceStdout)" ``` `==` rather than `~=`, so the two lanes land on the same three-component version rather than only the same minor line. `dependency:tree` puts the Flink dependencies carrying `flink.version` at 2.3.0 on both `api` and `runtime`, and pointed at a copy of the root POM with a different value the command follows it. I also applied it to `python-java-bridge.md`, which is already merged, since its Python bullet had the same split and two guides in one directory answering this differently seemed worse than the extra file. That does widen the PR into merged code though. Would you rather I split it out and send it separately? One cost worth flagging: that guide's parenthetical was the only pointer either guide had to the supported version set, and neither names it now. Worth restoring somewhere, or is one derived version per block the right scope here? I will open the `tools/ut.sh` issue and take a first pass at it. -- 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]
