SYaoJun commented on PR #966:
URL:
https://github.com/apache/incubator-graphar/pull/966#issuecomment-5571958947
Thanks for the PR — the module is cleanly separated (optional dependency,
caller-owned `S3Client`, conditional `PutObject`, eTag/version-pinned reads)
and the unit/IT split is well done. A few points should be resolved before this
is ready to merge:
**1. The new module is not covered by CI**
`.github/workflows/java-sdk.yml` does not trigger on, nor build,
`maven-projects/storage-s3`:
- `paths:` for both `push` and `pull_request` don't include
`maven-projects/storage-s3/**`, so the workflow didn't even run for this PR's
module.
- `SDK_MODULES: storage-api` means the compile/test jobs never touch
`storage-s3` (this PR only stayed green because the root
`maven-projects/pom.xml` changed).
Please add the module to the trigger paths and the module list, e.g.:
```yaml
paths:
- 'maven-projects/storage-s3/**'
...
env:
SDK_MODULES: storage-api,storage-local,storage-s3
```
(Also note `storage-local` from #960 is currently missing from
`SDK_MODULES`, so aligning this list fixes a pre-existing gap too.)
**2. `create()` fails late, unlike the local adapter — inconsistent behavior
under the same `Storage` API**
`LocalOutputFile.open()` fails immediately when the file exists
(`CREATE_NEW`), but `S3OutputFile.open()` only stages a temp file and defers
the existence check until `close()`'s conditional `PutObject` (`If-None-Match:
*`). A caller can end up buffering/writing the whole object before hitting the
"already exists" error. Suggestion: issue a `headObject` fast-fail in
`create()` at open time (covers the common case), and keep `If-None-Match` on
publish as the atomic backstop.
**3. Inconsistent license headers**
`S3InputFile.java` and `S3SeekableInput.java` use a single-line `/* ... */`
ASF header while every other file in the module (and in `storage-api` /
`storage-local`) uses the standard multi-line header. Please align those two
files with the canonical header format.
Happy to discuss any of these further. Thanks for the well-structured
contribution.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
