DanielLeens commented on PR #11967: URL: https://github.com/apache/seatunnel/pull/11967#issuecomment-5476960503
Thanks for checking back in, @SEZ9. Just to make sure we're not talking past each other: my previous review (posted a few hours before this one, on the same head `d4835e525870`) went through all 8 of these findings individually with source evidence, and I ended up in a different place on most of them — happy to recap so we're arguing from the same starting point: - **F8 (enum ordinal)** — resolved by the `dev` merge, verified against `origin/dev`'s current `FileSystemType.java` (GCS is appended last, no existing constant's ordinal moved). - **F1 (`hadoop_gcs_properties` passthrough)** and **F7 (non-atomic rename vs. `exactly-once`)** — I checked and both patterns are pre-existing and identical in `connector-file-s3` (`hadoop_s3_properties` is the same unrestricted map; S3A's `rename()` is also copy-then-delete with the same `exactly-once` claim). I don't think it's right to hold GCS to a stricter bar than the sibling connector it's modeled on for something this PR doesn't introduce — I'd rather see these filed as cross-cutting issues against `connector-file-base` / all cloud connectors. Downgraded to Low. - **F2 (SPI file collision)** — the root `pom.xml`'s shared `maven-shade-plugin` execution already configures `ServicesResourceTransformer`, which is exactly what merges colliding `META-INF/services/org.apache.hadoop.fs.FileSystem` files at shade time, and S3/OBS ship the identical hand-written SPI file for the identical reason. Downgraded to Low, "watch if the connector version bumps." - **F6 (no fail-fast validation of `service_account_key_file`)** — agreed this is real, rated Low-Medium, non-blocking. - **F3/F4 (license inventory)** and **F5 (no e2e for a real `gs://` read)** — these are the two I kept as genuine **Medium, blocking** issues, for the reasons in my last review: the `Dependency licenses` CI job has actually been `skipped` (not `success`) on every run since the original failure, so the `known-dependencies.txt` removal is asserted but not CI-proven; and `connector-file-s3-e2e` shows a credential-free Testcontainer e2e (MinIO for S3, `fake-gcs-server` would be the GCS equivalent) is the established in-repo pattern, not an optional nice-to-have gated behind a live cloud account. So my current position is: this PR is "ready to merge after fixes," with exactly two concrete blockers — an actual (non-skipped) run of the license check, and a no-credential e2e test — and the rest resolved or downgraded with the reasoning above. If you still see it differently on any of the downgraded points, I'm glad to keep digging, but it'd help to know specifically which piece of that counter-evidence you think doesn't hold (e.g., if you think `ServicesResourceTransformer` isn't actually wired into this module's effective shade config, or the S3 precedent doesn't actually apply here) — the two open blockers themselves I think we're already aligned on. -- 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]
