zeroshade commented on code in PR #1781:
URL: https://github.com/apache/iceberg-go/pull/1781#discussion_r3865722475
##########
table/scanner.go:
##########
@@ -742,6 +738,42 @@ func (scan *Scan) filterManifestsWithSchema(
return filtered, nil
}
+// manifestIOBatch shares a FileIO within a concurrency-sized batch. A new
+// batch loads through the factory again so factories that renew credentials
+// still get regular checkpoints without rebuilding the backend for every
+// manifest.
+type manifestIOBatch struct {
+ factory FSysF
+ limit int
+
+ mu sync.Mutex
+ fs io.IO
+ remaining int
+}
+
+func newManifestIOBatch(factory FSysF, limit int) *manifestIOBatch {
+ return &manifestIOBatch{factory: factory, limit: max(limit, 1)}
+}
+
+func (b *manifestIOBatch) acquire(ctx context.Context) (io.IO, error) {
Review Comment:
`acquire` relies on the factory itself to reject a canceled context. `FSysF`
receives a context but is not required to inspect it, so after one worker
fails, a queued worker can still invoke a context-agnostic factory and open
another manifest.
I reproduced this 10/10 times with concurrency 1: after manifest 0 fails,
factory calls increase from the expected 2 to 3 and manifest 1 is opened once.
The new cancellation test masks this because its fake factory explicitly
returns `ctx.Err()`.
Please have `manifestIOBatch.acquire` check cancellation itself after
acquiring the mutex and before returning cached IO or invoking the factory. The
regression should use a factory that ignores context so the collector—not the
test double—enforces the invariant.
--
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]
