laskoviymishka commented on code in PR #1961:
URL: https://github.com/apache/iceberg-go/pull/1961#discussion_r3890695581
##########
io/gocloud/blobfs/blob.go:
##########
@@ -274,7 +284,7 @@ func (bfs *BlobFileIO) Open(path string) (icebergio.File,
error) {
return &blobOpenFile{Reader: r, name: name, key: key, b: bfs, ctx:
bfs.ctx}, nil
}
-func (bfs *BlobFileIO) Remove(name string) error {
+func (bfs *FileIO) Remove(name string) error {
var err error
name, err = bfs.preprocess(name)
Review 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:
```go
key, err := bfs.preprocess(name)
if err != nil {
return &fs.PathError{Op: "remove", Path: name, Err: err}
}
```
##########
io/gocloud/compat_test.go:
##########
@@ -0,0 +1,100 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gocloud_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/iceberg-go/io"
+ "github.com/apache/iceberg-go/io/gocloud"
+ "github.com/apache/iceberg-go/io/gocloud/blobfs"
+ "github.com/apache/iceberg-go/io/gocloud/gcs"
+ "github.com/apache/iceberg-go/io/gocloud/s3"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// For callers who still have not migrated
+var (
+ _ *blobfs.FileIO = (*gocloud.BlobFileIO)(nil)
Review 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.
##########
io/gocloud/blobfs/blob.go:
##########
@@ -323,7 +333,7 @@ func (bfs *BlobFileIO) WriteFile(name string, content
[]byte) error {
//
// 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) {
Review 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.
##########
io/gocloud/blobfs/blob.go:
##########
@@ -83,18 +86,18 @@ var ErrEmptyObjectKey = errors.New("object key is empty")
// URI to access it; this backend does not route across authorities.
var ErrUnsupportedObjectAuthority = errors.New("object URI authority is not
supported by this FileIO")
-type objectLocation struct {
+type ObjectLocation struct {
Review 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.
##########
io/gocloud/blobfs/utils.go:
##########
@@ -15,11 +15,11 @@
// specific language governing permissions and limitations
// under the License.
-package gocloud
+package blobfs
import "strings"
-func propertiesWithPrefix(props map[string]string, prefix string)
map[string]string {
+func PropertiesWithPrefix(props map[string]string, prefix string)
map[string]string {
Review 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.
##########
catalog/hadoop/io.go:
##########
@@ -19,7 +19,7 @@ package hadoop
import (
icebergio "github.com/apache/iceberg-go/io"
- "github.com/apache/iceberg-go/io/gocloud"
+ "github.com/apache/iceberg-go/io/gocloud/blobfs"
Review 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?
##########
internal/schemes/schemes.go:
##########
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package schemes
+
+import "slices"
+
+var (
+ S3 = []string{"s3", "s3a", "s3n", "oss"}
+ GCS = []string{"gs"}
+ Azure = []string{"abfs", "abfss", "wasb", "wasbs"}
+)
+
+var byBackend = map[string][]string{
Review 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.
##########
io/gocloud/s3/register_test.go:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package s3_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/iceberg-go/internal/schemes"
+ "github.com/apache/iceberg-go/io"
+ _ "github.com/apache/iceberg-go/io/gocloud/s3"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestRegistersOnlyItsOwnSchemes(t *testing.T) {
+ assert.ElementsMatch(t, append([]string{"file", "", "mem"},
schemes.S3...), io.GetRegisteredSchemes())
Review 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.
##########
internal/schemes/schemes.go:
##########
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package schemes
+
+import "slices"
+
+var (
+ S3 = []string{"s3", "s3a", "s3n", "oss"}
Review 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.
##########
io/gocloud/azure/register_test.go:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package azure_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/iceberg-go/internal/schemes"
+ "github.com/apache/iceberg-go/io"
+ _ "github.com/apache/iceberg-go/io/gocloud/azure"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestRegistersOnlyItsOwnSchemes(t *testing.T) {
+ assert.ElementsMatch(t, append([]string{"file", "", "mem"},
schemes.Azure...), io.GetRegisteredSchemes())
+}
+
+func TestOtherCloudSchemesRemainUnregistered(t *testing.T) {
+ ctx := context.Background()
+
+ for _, tt := range []struct {
+ location string
+ errValue string
Review 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.
--
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]
