keksmd commented on PR #966:
URL:
https://github.com/apache/incubator-graphar/pull/966#issuecomment-5573362411
@SYaoJun thanks for the careful read. All three points are addressed in the
three commits I just pushed, and the design questions are answered below.
**1. CI coverage — fixed in `ci(java): build the SDK modules that the
workflow already watches`**
You are right, and the gap was wider than this module: the workflow already
*triggers* on `storage-local/**`, `io-api/**` and `core/**`, but `SDK_MODULES:
storage-api` meant none of them were ever compiled or tested upstream. That is
my own regression from #962, where `storage-api` genuinely was the only SDK
module at the time. The workflow now watches `maven-projects/storage-s3/**` as
well and builds `storage-api,storage-local,io-api,storage-s3`.
Verified locally with exactly the module set CI will use, reading the
surefire reports rather than the exit code:
```
graphar-storage-api ..... SeekableInputTest 3/0/0
graphar-storage-local ... LocalStorageTest 5/0/0
graphar-io-api .......... 35 tests across 7 classes, 0 failures, 0 errors
graphar-storage-s3 ...... S3StorageTest 2/0/0
```
**2. Late failure in `create()` — fixed in `fix(java): reject an existing S3
object before staging bytes`**
`create()` now heads the object at open time and throws
`FileAlreadyExistsException`, the same type the local adapter surfaces from
`CREATE_NEW`. The conditional `PutObject` stays as the atomic backstop, so a
head request that cannot answer (throttled, denied) does not turn into a
spurious conflict — it just falls through to publication. Covered by a new unit
test asserting that nothing is published when `create()` is rejected.
**3. License headers — fixed in `style(java): use the canonical ASF header
in the S3 input files`**
Both files now carry the standard multi-line header.
---
**On the design questions**
`info` is metadata only: it loads and saves `*.graph.yml` / `*.vertex.yml` /
`*.edge.yml` into `GraphInfo`, `VertexInfo`, `EdgeInfo`. Its `src/main/java`
tree is `loader`, `saver`, `yaml`, `type` — there is no code there that reads
or writes a data chunk, so there is nothing in it for an S3 adapter to build
on. The name `ReaderGraphInfoLoader` refers to a `java.io.Reader`, not to a
data reader.
The pure-Java SDK is being built as a stack of narrow layers, each merged as
its own slice:
```
info metadata: parse and serialize GraphAr YAML
storage-api bytes: URI, InputFile/OutputFile, SeekableInput/PositionOutput
(#958)
storage-local local filesystem backend
(#960)
storage-s3 S3 backend
(this PR)
io-api format: batches, schema, read and write requests
(#961, #971, #972)
core layout: chunk ranges, offsets, adjacency resolution
(#968)
```
The rule the layers follow is that storage knows only URIs and byte streams
— no GraphAr layout, no Parquet, no Hadoop, no Arrow, no query semantics. That
is what keeps a backend swappable: `S3Storage` implements the same `Storage`
interface as `LocalStorage`, so everything above it is unchanged whether a
graph lives on a disk or in a bucket, and the AWS SDK stays an optional
dependency that a user who does not need S3 never pulls in.
So this PR does not duplicate anything in `info`; it sits under it, and the
reader vertical that will consume both is the layer still to come.
@SemyonSinchenko a second pair of eyes on the Java side would be welcome, as
SYaoJun suggested.
--
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]
