Revanth14 commented on PR #1324: URL: https://github.com/apache/iceberg-go/pull/1324#issuecomment-4961369812
> This is close now — every substantive thing from round 1 has landed. UUIDv7 generation plus version validation, the `PlanFailedError`/`ErrPlanCancelled` contract so a failed or cancelled plan can't be mistaken for an empty scan, `SupportsRemoteScanPlanning` gating on all four endpoints, the context-based access-delegation suppression with composing options, and the tests I asked for (cancelled/failed round-trips, the invalid-key rejection, endpoint gating). Thanks for working through all of it. > > The one thing still open is the 404 error-type split I raised last round — and zeroshade re-flagged it on the 3rd, so I'd treat it as the blocker for this pass. `FetchPlanningResult` still collapses every 404 to `ErrPlanExpired`, and `PlanTableScan`/`FetchScanTasks` collapse theirs to `ErrNoSuchTable`, all on status code alone with `error.type` never consulted. The comment defers this to the polling layer, but this is the only place with the type info, and a poll loop treating `ErrPlanExpired` as retry-forever will spin indefinitely when the table's actually gone — Java and PyIceberg abort there. I left the concrete fix inline and pointed at zeroshade's type enumeration rather than restating it. > > Two small new things while I was in here: the `IdempotencyKey` doc still calls it a "retry key" even though a fresh key is generated per call (so nil-then-retry silently defeats server dedup), and the RoundTrip change now lets a per-request header override any session default including a future Authorization — safe today only because authManager runs afterward, which is worth a comment and a regression test. Plus one test nit on an unguarded `pfe.Detail` deref. > > Fix the 404 split and I'm happy to take another pass and approve. Thanks for the detailed re-review. All remaining items are now addressed: - Added REST `error.type` mapping for all three scan-planning call sites, distinguishing plan ID, plan task, table, and namespace 404s. - Added `ErrNoSuchPlanTask`; bare and unknown 404s conservatively remain `ErrRESTError`. - Added typed error-body tests for every expected mapping and fallback case. - Reworded the idempotency-key documentation to clarify that `nil` generates a fresh key per call and explicit keys must be reused across retries. - Documented the load-bearing `authManager` ordering and added a regression test confirming managed authorization wins. - Added the missing `pfe.Detail` nil guard. - Kept `SupportsRemoteScanPlanning` false while `PlanFiles` remains unimplemented. -- 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]
