section9-lab commented on PR #718: URL: https://github.com/apache/flink-agents/pull/718#issuecomment-4590543363
Thanks for the thorough review @weiqingy! All five inline comments are addressed in ff6da88 — replies inline on each. Two summary items for the top-level comment: ### 1. `dist/pom.xml` registration Added gemini to the dist dependency list alongside the other connectors. Verified by clean-rebuilding `dist/flink-2.2` and checking the produced JARs: - `GeminiChatModelConnection`/`GeminiChatModelSetup` classes are present in both thin and fat jars ✓ - `com/google/genai/*` SDK classes: 3363 ✓ - `com/google/auth/*`: 188 ✓ ; `io/grpc/*`: 4003 ✓ ; `com/google/protobuf/*`: 1474 ✓ ### 2. Shade impact (the relocation/duplicate-class question) Audited the shade output. A few data points: **Fat-jar size.** `dist/common` grows from ~118 MB to ~225 MB (+104 MB). The delta is exactly the transitive set you flagged: `google-genai` + `protobuf-java` + `grpc` + `google-auth` + Apache `httpclient`. Not small, but not pathological for an LLM SDK either. **Duplicate-class warnings.** Total class-overlap warnings: ~41 (vs. ~38 pre-existing without gemini). The single largest new overlap is at `dist/flink-2.x` shade time: > `flink-agents-dist-common.jar, google-genai-1.56.0.jar define 3323 overlapping classes and resources` This is the **same structural pattern that already exists for ollama4j** (`common + ollama4j → 90 overlapping`): `dist/flink-2.x` shades `common` (which already contains the transitive deps) AND the transitive deps directly. It's last-write-wins on identical classes, so no runtime breakage — but it does inflate jar size. **No new cross-vendor conflicts.** google-genai's transitive guava/jackson/error_prone/etc. all align with versions already in the reactor. NOTICE/LICENSE entries are overlapping (same as every connector) but collapsed by the existing `ManifestResourceTransformer`. **My suggestion:** the cleanup belongs in `dist/flink-2.x` shade config (exclude transitive-deps-already-in-common from being pulled in twice), and it applies to ollama4j today too — so I'd treat it as a follow-up cleanup rather than gating this PR on it, unless you'd prefer I include it here. ### Other changes since the previous push While addressing the review I also: - Switched the default model from `gemini-3-pro-preview` to `gemini-3.1-pro-preview`, because the former is now deprecated on the live Google Developer API (404 "no longer available" when I ran a direct e2e against `generativelanguage.googleapis.com`). - Clamped the HttpOptions timeout computation in `long` to avoid int overflow. - Made `convertResponse` raise an explicit `IllegalStateException` on empty candidates (safety-block / quota), instead of silently returning an empty assistant message. Matches the Anthropic connector's contract. ### E2E Verified the full chain (text → function call → thoughtSignature echo → tool-result replay → final answer) directly against the real Google Developer API endpoint with `gemini-3.1-pro-preview`. No proxy in the path this time. PTAL! -- 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]
