weiqingy commented on code in PR #961:
URL: https://github.com/apache/flink-agents/pull/961#discussion_r3755203442
##########
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:
Pinned to `apache-flink~=2.3.0`.
I anchored it to the root `pom.xml`'s `flink.version` rather than
`tools/ut.sh`, and would welcome a second opinion. The Java command directly
above resolves 2.3.0 through `${flink.version}`, so ut.sh's 2.2 would have
split the two lanes. `tools/ut.sh` also seems to disagree with itself: `:24`
sets 2.2, while the help text at `:48` says it runs all versions. Worth a
separate issue, or have I misread it?
The pinned lane resolves 2.3.0 and reports the same 450 passed and 11
skipped as 2.2.1, so it bounds the install without narrowing what is known to
work.
One limit: nothing greps `review-guides/` for version strings, so this will
not follow a future bump on its own. If you would rather derive it the way
`python-java-bridge.md` does, without a number, that is an easy trade.
--
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]
