Repository navigation
refactor(hadoop)!: split catalog operations and preserve initialization context - #2290
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The split into tables.go and namespaces.go looks clean: apart from the new file headers, only the context lines changed. One blocker, though: io.LoadFS(ctx, …) doesn't just bound setup. For object stores, blobfs.New keeps ctx as the FileIO's lifetime context (io/gocloud/blobfs/blob.go:388), and every later write, delete, list and open uses it. So a catalog built under a timeout or request context, e.g. ctx, cancel := context.WithTimeout(...); cat, _ := catalog.Load(ctx, "hadoop", props); cancel(), fails every S3/GCS/Azure operation afterwards, commits included. I confirmed WriteFile returns context canceled once a blobfs FileIO's context is cancelled.
Could you load the filesystem with context.WithoutCancel(ctx) and check ctx.Err() yourself for setup cancellation, as catalog/rest/vended_creds.go:199-203 does? A test that builds the catalog under a context, cancels it, and then writes would pin this. A local warehouse won't show it, since LocalFS ignores contexts.
laskoviymishka
left a comment
There was a problem hiding this comment.
+1 to @zeroshade here. At e778527 the filesystem loads under context.WithoutCancel(ctx), and TestNewCatalogFilesystemContextLifetime fails with that removed. I'll leave the re-review to him.
| return updated, newMetaPath, nil | ||
| } | ||
|
|
||
| func (c *Catalog) commitMetadataFile(ident table.Identifier, version int, tempPath, metaPath string, conflictErr error) error { |
There was a problem hiding this comment.
My #2067 adds ctx to io.IO.Open/Remove, and 7 of its 8 hadoop.go hunks are in CreateTable, CommitTable, commitMetadataFile and DropNamespace, which this PR moves here and into namespaces.go unchanged. The two conflict in hadoop.go, so whichever lands second has to re-apply the Remove(ctx, ...) and commitMetadataFile(ctx, ...) edits in the new files by hand.
…on context Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
6c7323c to
476c9c1
Compare
Port apache#2067's context changes (ctx-aware Remove, cancellation-immune cleanup in commitMetadataFile, DropNamespace ctx) into the moved tables.go and namespaces.go.
zeroshade
left a comment
There was a problem hiding this comment.
Thanks, this fixes the lifetime issue: NewCatalog checks ctx.Err() around setup and loads the filesystem with context.WithoutCancel(ctx), and TestNewCatalogFilesystemContextLifetime pins it. It fails for both the direct constructor and the registry path if LoadFS gets the caller's ctx.
#2067 merged after your last update and changed commitMetadataFile, CreateTable, CommitTable and DropNamespace in hadoop.go (ctx-aware Remove, cancellation-immune cleanup). Since this PR moves those functions, I pushed a merge of main (fde19f4) that ports those changes into tables.go and namespaces.go. A sorted-line diff against main's hadoop.go now shows only this PR's intended context changes.
Nit: hadoop.NewCatalog's new ctx parameter breaks callers but has no entry under "Unreleased compatibility notes" in releases.md. Could you add one in a follow-up? LGTM.
…pache#2307, apache#2310 and apache#2313 Requested in review: these merged without compatibility notes.
…g changes Add release notes for the rename batch (apache#2308, apache#2314, apache#2315, apache#2316, apache#2317, apache#2319, apache#2322, apache#2323) and for apache#2067, apache#2290, apache#2294, apache#2307, apache#2310 and apache#2313. Remove stray blank lines inside the compatibility-notes list.
What
Split Hadoop catalog table and namespace operations into separate files. Pass the caller context during filesystem setup.
Why
Hadoop catalog initialization currently ignores caller cancellation while loading its filesystem.
Implementation
hadoop.NewCatalogandio.LoadFS.Breaking change:
hadoop.NewCatalognow requires a context.