wenjin272 commented on code in PR #961: URL: https://github.com/apache/flink-agents/pull/961#discussion_r3747311926
########## 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. Review Comment: This feels a little too absolute. The repository currently has no formal deprecation process, but under the beta policy the key question is whether there is a concrete compatibility obligation. Could we instead say that reviewers should prefer complete removal when no such obligation exists? ########## 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: Could we avoid installing an unpinned latest `apache-flink` here? This validation command may become unstable when a newer unsupported version is released. It would be safer to use a repository-supported version or derive one from the test matrix/default. -- 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]
