feat(io): split io/gocloud into per-cloud backend packages - #1961
Conversation
Signed-off-by: badalprasadsingh <badal@datazip.io>
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Really like the shape of this. The dependency-free blobfs base with per-cloud registration packages is the right decomposition, and keeping io/gocloud as a type-alias + blank-import shim means existing callers don't break on the move.
I'd hold it before merging though, because this reshapes the public io surface and a few follow-ups will build on whatever contract lands here, so I'd like that contract to be solid first.
The one that matters most is catalog/hadoop. It used to pull in io/gocloud's init() transitively, so importing the hadoop catalog registered all nine cloud schemes for free. Now it imports only blobfs, which has no registration, so a program that imports just catalog/hadoop and uses s3/gcs/azure will build fine and then fail at the first LoadFS with ErrIOSchemeNotFound after upgrading. I don't think we should re-link all three SDKs into hadoop to paper over it, but we also shouldn't let it break silently: either keep the blank import or make it a loud, documented break. And the compat shim only tests type aliases today, not the "blank-importing io/gocloud registers all nine schemes" promise that's its entire reason to exist.
Things I'd like to settle in this PR before the follow-ups build on it:
- a deliberate decision on catalog/hadoop registration, reflected in the CHANGELOG and package doc rather than only the website config page
- a test that the shim actually registers all nine schemes, plus one for the catalog/hadoop behavior after that decision
- the newly-exported blobfs surface (ObjectLocation with no accessors, PropertiesWithPrefix) settled as either a usable and documented public API or kept internal
Once those land I'm happy to take another pass and approve. Thanks for taking this on, it's a good cleanup.
| import ( | ||
| icebergio "github.com/apache/iceberg-go/io" | ||
| "github.com/apache/iceberg-go/io/gocloud" | ||
| "github.com/apache/iceberg-go/io/gocloud/blobfs" |
There was a problem hiding this comment.
I'd hold on this import swap until we decide what happens to scheme registration for catalog/hadoop users.
Today the value import of io/gocloud pulls in that package's init(), so anyone importing catalog/hadoop gets all nine cloud schemes registered for free. Swapping to blobfs (which has no registration init()) means a program that imports only catalog/hadoop and talks to s3/gcs/azure will now build fine and then fail at the first LoadFS with ErrIOSchemeNotFound. That's a silent runtime regression on go get -u, with no compile-time signal.
I think the split itself is right, so I wouldn't want to re-link all three SDKs into hadoop just to preserve the old behavior. But we shouldn't let it break silently either. Either keep a blank import of io/gocloud here, or treat this as an intentional break and call it out loudly in the CHANGELOG and package doc (the website config page alone won't reach someone upgrading). wdyt?
|
|
||
| // For callers who still have not migrated | ||
| var ( | ||
| _ *blobfs.FileIO = (*gocloud.BlobFileIO)(nil) |
There was a problem hiding this comment.
The compat shim's whole contract is that blank-importing io/gocloud still registers all nine schemes, but this test only checks the type aliases and the wrapper functions.
If a future edit drops one of the sub-package imports from gocloud.go, the aliases still compile (they live in blobfs) and this test stays green, but the registration side effect silently disappears. I'd add an assertion that io.GetRegisteredSchemes() contains all nine cloud schemes after importing io/gocloud, mirroring the per-backend TestRegistersOnlyItsOwnSchemes.
While we're at it, the flip side of the catalog/hadoop change has no test either. A small case in catalog/hadoop that imports it without any backend and asserts LoadFS(ctx, nil, "s3://...") returns ErrIOSchemeNotFound would lock in whatever we decide above.
| func (bfs *BlobFileIO) Remove(name string) error { | ||
| func (bfs *FileIO) Remove(name string) error { | ||
| var err error | ||
| name, err = bfs.preprocess(name) |
There was a problem hiding this comment.
While we're moving this into a public package, worth fixing: Remove reassigns name with the preprocessed key, so when preprocess fails and returns "", the &fs.PathError{Path: name} below reports an empty path instead of the caller's original.
Open already guards against this with originalPath := path. Same gap in WriteFile and NewWriter. I'd take a separate key variable and leave name/path intact:
key, err := bfs.preprocess(name)
if err != nil {
return &fs.PathError{Op: "remove", Path: name, Err: err}
}| var ErrUnsupportedObjectAuthority = errors.New("object URI authority is not supported by this FileIO") | ||
|
|
||
| type objectLocation struct { | ||
| type ObjectLocation struct { |
There was a problem hiding this comment.
Now that ObjectLocation is exported, its fields are all still unexported and the only constructor is NewObjectLocation. An external package writing a custom ObjectLocationExtractor can build one but can't read scheme/authority/key back off a value it's handed, which limits how far a custom extractor can route.
I'd either add Scheme()/Authority()/Key() accessors, or, if it's meant to be opaque, say so in a doc comment and point people at KeyExtractorFromObjectLocation. Either way these newly-exported symbols (ObjectLocation, NewObjectLocation, ObjectLocationExtractor, KeyExtractorFromObjectLocation, DefaultObjectLocationExtractor) are the extension surface for custom backends and none of them have doc comments yet.
| // The caller must call Close on the returned Writer, even if the write is | ||
| // aborted. | ||
| func (bfs *BlobFileIO) NewWriter(ctx context.Context, path string, overwrite bool, opts *blob.WriterOptions) (w *blobWriteFile, err error) { | ||
| func (bfs *FileIO) NewWriter(ctx context.Context, path string, overwrite bool, opts *blob.WriterOptions) (w *blobWriteFile, err error) { |
There was a problem hiding this comment.
NewWriter is exported on an exported type but returns *blobWriteFile, which callers outside the package can't name. It's valid Go and predates the split, but the rename makes it public surface now, so I'd either return an exported Writer interface or export the concrete type.
| Azure = []string{"abfs", "abfss", "wasb", "wasbs"} | ||
| ) | ||
|
|
||
| var byBackend = map[string][]string{ |
There was a problem hiding this comment.
One subtle thing here: byBackend copies the S3/GCS/Azure slice headers at package load. Since these are exported mutable vars, if anything ever appends to one past its capacity, byBackend keeps pointing at the old backing array and BackendFor silently returns "" for the new scheme.
It's internal so the blast radius is small, but adding a scheme alias looks like a harmless one-liner. I'd at least add a doc comment noting these are read-only, or build byBackend lazily so it can't drift.
| import "strings" | ||
|
|
||
| func propertiesWithPrefix(props map[string]string, prefix string) map[string]string { | ||
| func PropertiesWithPrefix(props map[string]string, prefix string) map[string]string { |
There was a problem hiding this comment.
This is a generic map-prefix helper with no relationship to blob FS, and its only caller is azure.go. Exporting it from blobfs makes it permanent public API.
I'd keep it unexported (the azure package can hold its own two-line copy) or move it to an internal/ helper, rather than committing to it as public surface. If it does stay exported, it needs a doc comment on what it matches and whether the prefix is stripped from the keys.
| ) | ||
|
|
||
| func TestRegistersOnlyItsOwnSchemes(t *testing.T) { | ||
| assert.ElementsMatch(t, append([]string{"file", "", "mem"}, schemes.S3...), io.GetRegisteredSchemes()) |
There was a problem hiding this comment.
The "mem" here comes from a transitive import rather than anything in this package, so if that dependency changes this fails with no obvious reason. A one-line comment naming where mem gets registered would save the next person the hunt.
The exact-set match via ElementsMatch also means any newly-registered legit scheme breaks all three of these. assert.Subset for the cloud schemes you actually care about would be less brittle. Same pattern in the gcs and azure register tests.
| import "slices" | ||
|
|
||
| var ( | ||
| S3 = []string{"s3", "s3a", "s3n", "oss"} |
There was a problem hiding this comment.
Worth a doc comment on this slice: oss routes through the AWS S3 SDK here, which works via OSS's S3-compatible API but diverges from Java (dedicated OSSFileIO) and means OSS users configure s3.* credential keys, not oss.*. Easy to trip over otherwise.
|
|
||
| for _, tt := range []struct { | ||
| location string | ||
| errValue string |
There was a problem hiding this comment.
Small consistency thing: the s3 and gcs register tests call this field wantHint; only azure uses errValue. I'd rename to wantHint to match.
Signed-off-by: badalprasadsingh <badal@datazip.io>
Signed-off-by: badalprasadsingh <badal@datazip.io>
2 participants
Solves #1874, not adding the "Fixes" tag as leaving upon the user to close the issue once the PR is merged :)
The implementation is based on the Proposal shared earlier.
Following this PR,
io/gocloudis split intoblobfs, a bucket-basedFileIOwith no cloud SDK dependency, plus per-clouds3,gcsandazureregistration packages withio/gocloudbehaving as an aggregator.Changelog Note
io/gocloudis split into per-cloud backend packages. Blank-importingio/gocloudstill registers all nine schemes. Its exported symbols remain available asDeprecated:aliases.catalog/hadoopno longer registers cloud schemes. It previously did so as a side effect if importingio/gocloud, which linked all three cloud SDKs into every binary using the catalog. Hadoop-catalog users on cloud storage must blank-import the matching backend, otherwise the firstLoadFSfor a cloud path fails withErrIOSchemeNotFoundnaming the package to import. This is recorded in thecatalog/hadooppackage doc so it is visible fromgo docon upgrade.Public API
blobfsis now the extension point for custom backends.ObjectLocation(withScheme,AuthorityandKeyaccessors),NewObjectLocation,ObjectLocationExtractor,KeyExtractorFromObjectLocation,DefaultObjectLocationExtractorandWriterare exported and documented.