tanmayrauth commented on code in PR #1286:
URL: https://github.com/apache/iceberg-go/pull/1286#discussion_r3464696509
##########
table/orphan_cleanup.go:
##########
@@ -424,64 +422,7 @@ func walkDirectory(fsys iceio.IO, root string, fn
func(path string, info stdfs.F
})
}
- // Fallback to original implementation for IO types that don't
- // implement ListableIO yet.
- 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 = "."
- }
-
- return stdfs.WalkDir(bucket, walkPath, makeFileWalkFunc(fn,
func(path string) string {
- return parsed.Scheme + "://" + parsed.Host + "/" + path
- }))
- }
-}
-
-// getBucketName gets the Bucket field from blob storage - absolute minimal
approach.
-func getBucketName(fsys iceio.IO) stdfs.FS {
- v := reflect.ValueOf(fsys).Elem()
-
- return v.FieldByName("Bucket").Interface().(stdfs.FS)
-}
-
-// makeFileWalkFunc creates a WalkDirFunc that processes only files with path
transformation.
-func makeFileWalkFunc(fn func(path string, info stdfs.FileInfo) error,
pathTransform func(string) string) stdfs.WalkDirFunc {
- return func(path string, d stdfs.DirEntry, err error) error {
- if err != nil {
- return err
- }
-
- if d.IsDir() {
- return nil
- }
-
- info, err := d.Info()
- if err != nil {
- return err
- }
-
- return fn(pathTransform(path), info)
- }
+ return fmt.Errorf("filesystem %T does not implement io.ListableIO",
fsys)
Review Comment:
Minor consistency thought, not blocking: PurgeFiles at line 780 silently
skips the walk when the FS isn't listable, while this now returns a hard error.
I think erroring is the right call for orphan cleanup (vs. best-effort purge)
--
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]
