tanmayrauth commented on code in PR #916:
URL: https://github.com/apache/iceberg-go/pull/916#discussion_r3101818328
##########
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:
Pinned both in the interface doc:
- Missing files: "not considered errors — if a path does not exist, it
should be treated as a successful deletion" (matches S3 DeleteObjects behavior;
backends like os.Remove should swallow ErrNotExist)
- Error shape: "a joined error (via errors.Join) for any individual
failures" — callers can errors.As / errors.Unwrap to get per-file errors
##########
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:
Changed return signature to (deleted []string, err error). Mirrors the
sequential contract — callers can distinguish partial success from total
failure, enabling resumability. Since the caller wiring is deferred to #917,
this will be exercised end-to-end there.
##########
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:
Renamed to DeleteFiles(ctx context.Context, paths []string) (deleted
[]string, err error). Unambiguous — it deletes files from a list, doesn't
collide with os.RemoveAll, and the plural makes it clear it's not a single-path
recursive delete.
--
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]
