Revanth14 commented on PR #1286: URL: https://github.com/apache/iceberg-go/pull/1286#issuecomment-4793508574
> This is a clean refactor and the core claim holds up — LocalFS, MemFS, and blobFileIO behind every gocloud scheme all implement WalkDir, so the fallback was genuinely dead and dropping the reflection-based getBucketName is a nice cleanup. > > I'd hold it before merging on two things. The error string says `io.ListableIO`, but the real type is `iceio.ListableIO` and stdlib `io` is also in scope — anyone hitting that error will go looking in the wrong package. And with the fallback gone, the only unit test on `walkDirectory` covers the error branch; the success path has no coverage, so a regression in the ListableIO callback would slip through normal CI. I left an inline suggesting a MemFS-driven case. > > One design point worth a thought, not blocking: `DeleteOrphanFiles` and `FSysF` are public, and this makes a custom IO that implements only `iceio.IO` fail hard at runtime. A line of godoc on `DeleteOrphanFiles` would close that gap, and it'd be worth aligning with `PurgeFiles`, which handles the same condition by silently skipping the walk. > > Once the error string and the test gap are addressed, happy to take another pass and approve. Thanks for the review. Addressed the two blocking items: - Updated the error string and assertion from `io.ListableIO` to `iceio.ListableIO`. - Added `TestWalkDirectoryUsesListableIO` to cover the success path through `walkDirectory`. Also added the `DeleteOrphanFiles` godoc note and the `blobFileIO` compile-time `ListableIO` assertion from the follow-up comments. I left `PurgeFiles` behavior unchanged because purge is best-effort after catalog drop, while orphan cleanup needs complete location enumeration before deciding what is safe to 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]
