badalprasadsingh commented on code in PR #2451:
URL: https://github.com/apache/iceberg-go/pull/2451#discussion_r4533795267
##########
io/gocloud/blobfs/blob_test.go:
##########
@@ -1063,13 +1063,35 @@ func TestBlobFileIOMkdirAll(t *testing.T) {
bfs := New(ctx, bucket, DefaultObjectLocationExtractor("test-bucket"))
- require.NoError(t, bfs.MkdirAll("s3://test-bucket/a/b/c"))
+ require.NoError(t, bfs.MkdirAll(ctx, "s3://test-bucket/a/b/c"))
for _, key := range []string{"a/", "a/b/", "a/b/c/"} {
exists, err := bucket.Exists(ctx, key)
require.NoError(t, err)
assert.True(t, exists, "%s should exist", key)
}
+ require.NoError(t, bfs.MkdirAll(ctx, "s3://test-bucket/a/b/c"))
+}
+
+func TestBlobFileIOMkdirAllUsesOperationContext(t *testing.T) {
+ ctx := context.Background()
+ bucket := memblob.OpenBucket(nil)
+ defer bucket.Close()
+ bfs := testBlobFileIO(ctx, "test-bucket", bucket)
+
+ opCtx, cancel := context.WithCancel(ctx)
Review Comment:
this test and the `RemoveAll` one cancel the context first, so the new
`ctx.Err()` checks return before any bucket call, no test checks that
`WriteAll`, `Delete` and `iter.Next` get the caller's context
better: add `MkdirAll` to
`TestBlobFileIOWriteOperationsUseOperationContext`, and test RemoveAll with a
recording bucket like `statListContextBucket`
##########
catalog/hadoop/hadoop_context_test.go:
##########
@@ -61,12 +61,15 @@ func (f *contextBoundHadoopFS) Stat(ctx context.Context,
name string) (fs.FileIn
return f.LocalFS.Stat(ctx, f.localPath(name))
}
-func (f *contextBoundHadoopFS) MkdirAll(name string) error {
+func (f *contextBoundHadoopFS) MkdirAll(ctx context.Context, name string)
error {
if err := f.ctx.Err(); err != nil {
return err
}
+ if err := ctx.Err(); err != nil {
+ return err
+ }
Review Comment:
no test checks that CreateNamespace, CreateTable, CommitTable, DropTable and
PurgeTable pass the caller's context to MkdirAll or RemoveAll, and this new
check is never reached: a canceled CreateNamespace fails at Stat first
better: drop it, wrap `cat.filesystem` as `failingWalkFS` does, and assert
the `ctx.Value` both methods receive.
--
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]
