laskoviymishka commented on code in PR #916:
URL: https://github.com/apache/iceberg-go/pull/916#discussion_r3100259838
##########
io/io.go:
##########
@@ -90,6 +90,31 @@ type WriteFileIO interface {
WriteFile(name string, p []byte) error
}
+// BulkRemovableIO is an optional interface for IO implementations that
+// support deleting multiple files in a single batch operation.
+// Cloud object stores (S3 DeleteObjects, GCS batch, Azure batch) can
+// implement this for significantly better throughput than single-file Remove.
+type BulkRemovableIO interface {
+ IO
+
+ // RemoveAll deletes all named files. Implementations should make a
+ // best-effort attempt to delete as many files as possible and return
+ // a joined error for any individual failures.
+ RemoveAll(paths []string) error
Review Comment:
`os.RemoveAll(path)` recursively removes a directory tree. This
`RemoveAll(paths []string)` removes individual files from a list. Same name,
different semantics — a reader can easily mis-read the call site as "recursive
delete."
`RemoveBatch` or `DeleteMany` would avoid the ambiguity. Any preference?
##########
io/io.go:
##########
@@ -90,6 +90,31 @@ type WriteFileIO interface {
WriteFile(name string, p []byte) error
}
+// BulkRemovableIO is an optional interface for IO implementations that
+// support deleting multiple files in a single batch operation.
+// Cloud object stores (S3 DeleteObjects, GCS batch, Azure batch) can
+// implement this for significantly better throughput than single-file Remove.
+type BulkRemovableIO interface {
Review Comment:
Adding `ctx` after the interface ships is a breaking change for every
implementer.
Any reason to skip it now?
##########
io/io.go:
##########
@@ -90,6 +90,31 @@ type WriteFileIO interface {
WriteFile(name string, p []byte) error
}
+// BulkRemovableIO is an optional interface for IO implementations that
+// support deleting multiple files in a single batch operation.
+// Cloud object stores (S3 DeleteObjects, GCS batch, Azure batch) can
+// implement this for significantly better throughput than single-file Remove.
+type BulkRemovableIO interface {
+ IO
+
+ // RemoveAll deletes all named files. Implementations should make a
+ // best-effort attempt to delete as many files as possible and return
+ // a joined error for any individual failures.
+ RemoveAll(paths []string) error
Review Comment:
Doc says "best-effort, joined error" but two things are still open:
- Missing files — error or silent success? S3 `DeleteObjects` silently
succeeds; `os.Remove` errors. Backends will pick different answers.
- Returned error shape — required to be an `errors.Join` so callers can
unwrap per-file errors?
Worth pinning in the doc before there are multiple backends to reconcile.
##########
table/updates.go:
##########
@@ -534,6 +535,15 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx
context.Context, preTable *Table,
delete(filesToDelete, psf.StatisticsPath)
}
+ if bulk, ok := prefs.(io.BulkRemovableIO); ok {
Review Comment:
Three things:
1. `ctx` is ignored (same as the interface issue above).
2. `return` short-circuits — if `RemoveAll` errors, the per-file fallback
below is skipped. Is that intentional, or should a failed bulk fall through to
the loop?
3. `paths` can be empty (all snapshot files still referenced). S3
`DeleteObjects` errors on empty batches. One-line guard avoids that.
##########
table/orphan_cleanup.go:
##########
@@ -333,37 +325,15 @@ func makeFileWalkFunc(fn func(path string, info
stdfs.FileInfo) error, pathTrans
}
}
-func walkDirectory(fsys iceio.IO, root string, fn func(path string, info
stdfs.FileInfo) error) error {
- switch v := fsys.(type) {
- case iceio.LocalFS:
- cleanRoot := strings.TrimPrefix(root, "file://")
- if cleanRoot == "" {
- cleanRoot = "."
- }
-
- return filepath.WalkDir(cleanRoot, makeFileWalkFunc(fn,
func(path string) string {
- return path
- }))
-
- default:
- // For blob storage: direct field access since we know the
structure
- bucket := getBucketName(v)
-
- parsed, err := url.Parse(root)
- if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
-
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
- walkPath = "."
- }
-
- // URL transform - reconstruct full URL path
- return stdfs.WalkDir(bucket, walkPath, makeFileWalkFunc(fn,
func(path string) string {
- return parsed.Scheme + "://" + parsed.Host + "/" + path
- }))
+func walkDirectory(fsys io.IO, root string, fn func(path string, info
stdfs.FileInfo) error) error {
Review Comment:
The new `walkDirectory` requires `ListableIO`. `LocalFS` gets `WalkDir` in
this PR, but `blobFileIO` (S3/GCS/Azure/mem) does not, that work lives in #917.
Merging #916 alone means every call to `DeleteOrphanFiles` against a cloud
backend fails with `"IO implementation *blobFileIO does not support directory
listing (ListableIO)"`. That's a functional regression for anyone using orphan
cleanup on object storage today.
Should we merge those pr together, or land atomically with #917?
##########
io/io.go:
##########
@@ -90,6 +90,31 @@ type WriteFileIO interface {
WriteFile(name string, p []byte) error
}
+// BulkRemovableIO is an optional interface for IO implementations that
Review Comment:
`gocloud.dev/blob` doesn't expose a native batched delete — its `Delete(ctx,
key)` is single-key only. No backend in this repo implements `BulkRemovableIO`,
so the fast path in `deleteFiles`/`PostCommit` is unreachable for every shipped
IO today.
If the plan is "define the interface, wire a backend in a follow-up," the
caller wiring probably shouldn't land until that follow-up, no backend means
the contract hasn't been validated against a real implementation.
What's the sequencing plan here?
##########
table/orphan_cleanup.go:
##########
@@ -410,19 +380,31 @@ func isFileOrphan(file string, referencedFiles
map[string]bool, normalizedRefere
return true, nil
}
-func deleteFiles(fs iceio.IO, orphanFiles []string, cfg *orphanCleanupConfig)
([]string, error) {
+func deleteFiles(fs io.IO, orphanFiles []string, cfg *orphanCleanupConfig)
([]string, error) {
if len(orphanFiles) == 0 {
return nil, nil
}
+ // Use bulk delete when available and no custom deleteFunc is set.
+ if cfg.deleteFunc == nil {
+ if bulk, ok := fs.(io.BulkRemovableIO); ok {
+ if err := bulk.RemoveAll(orphanFiles); err != nil {
Review Comment:
The sequential path carefully builds `deletedFiles` as each file succeeds
and joins per-file errors. The bulk path returns `nil` for deleted files on any
error, even though the interface doc says implementations "make a best-effort
attempt to delete as many files as possible."
A caller can't distinguish "nothing was deleted" from "999/1000 succeeded,"
which matters for resumability and stats. Either mirror the sequential contract
(return what succeeded) or change the signature to `RemoveAll(paths) (deleted
[]string, err error)`?
--
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]
