refactor(io): use the registry pattern for IO schemes by alessandro-nori · Pull Request #709 · apache/iceberg-go

alessandro-nori · 2026-02-02T11:04:28Z

Related to #696

This PR introduces a registry pattern for IO implementations, similar to the existing catalog package pattern.

Moved all cloud storage implementations to io/gocloud.

Extra notes

I decided to use a single subpackage because all the existing implementations use the same dependency and it's easier to import just one package to register all of them. However I think in most of the integration tests only use s3 so multiple subpackages would also work fine.

alexandre-normand · 2026-02-04T17:09:07Z

io/registry.go

+	if factory == nil {
+		panic("io: Register factory is nil")
+	}
+	defaultRegistry.set(scheme, factory)


Should we have something here to handle a case where there's already something registered for a given scheme? In this current version, it looks like a user importing two packages registering themselves on the same scheme could result in a surprising behavior.

For reference, database/sql actually checks for prior existence of a driver and panics in case of a duplicate registration.

That's a fair point. I just mirrored what was done in the catalog package but I think this is worth adding here

This makes sense, currently the stated semantics of the catalog registry is that if something is registered it will overwrite anything that is already registered with that catalog type. but for file IO it might makes sense to be a bit more strict

Thanks Alex and Matt!
I implemented the change in ec4790b

Does it make sense to handle it atomically? Because right now, two goroutines can register same scheme while skipping panic

One solution would be for the mutex to be in the Register and Unregister functions rather than in the set and remove methods of the registry object.

zeroshade · 2026-02-05T21:33:28Z

catalog/rest/rest_integration_test.go

+		gocloud.S3Region:          "us-east-1",
+		gocloud.S3AccessKeyID:     "admin",
+		gocloud.S3SecretAccessKey: "password",


since these are properties that tend to be shared across any IO implementation, we should probably leave these constants in the io package rather than moving them down to the gocloud package. the assumption being that the properties being looked for in the catalog should not be dependant on the IO implementation being used.

Done in e18695d
🙇

catalog/sql/sql_integration_test.go

io/gocloud/azure_integration_test.go

zeroshade · 2026-02-05T21:39:06Z

io/gocloud/register.go

+	icebergio.Register("mem", func(ctx context.Context, parsed *url.URL, props map[string]string) (icebergio.IO, error) {
+		bucket := memblob.OpenBucket(nil)
+
+		return createBlobFS(ctx, bucket, defaultKeyExtractor(parsed.Host)), nil
+	})


I don't think we had any tests for mem, can we add some?

Sure, I added some basic tests to create, write, read and delete an in-memory file
068d8cf

ferhatelmas · 2026-02-09T21:58:53Z

io/gocloud/azure.go

 // under the License.

-package io
+package gocloud


Due to nesting, all integration tests aren't running in CI anymore

Thanks 😬 ! Fixed it in 4556800

Looks good 👍🏻

zeroshade

Just the one nitpick on atomically handling register/unregister

Otherwise this looks good to me!

ferhatelmas · 2026-02-12T20:07:44Z

io/registry.go

+type registry map[string]SchemeFactory
+
+var (
+	regMutex        sync.Mutex


it might be better to use sync.RWMutex

ferhatelmas · 2026-02-12T20:12:14Z

io/io.go

 }

 var (
+	ErrIONotFound = errors.New("io scheme not registered")


ErrIOSchemeNotFound?

ferhatelmas · 2026-02-12T20:14:11Z

io/registry.go

+	regMutex.Unlock()
+
+	if !ok {
+		return nil, fmt.Errorf("%w: %s", ErrIONotFound, parsed.Scheme)


Previously path was added into error but now it's removed. Is this a change we want? It could be useful for debugging

I agree, let's keep the path in the error

ferhatelmas · 2026-02-13T22:45:15Z

io/config.go

+	S3EndpointURL            = "s3.endpoint"
+	S3ProxyURI               = "s3.proxy-uri"
+	S3ConnectTimeout         = "s3.connect-timeout"
+	S3SignerUri              = "s3.signer.uri"


Suggested change

S3SignerUri = "s3.signer.uri"

S3SignerURI = "s3.signer.uri"

ferhatelmas · 2026-02-13T22:45:34Z

io/config.go

+	GCSKeyPath    = "gcs.keypath"
+	GCSJSONKey    = "gcs.jsonkey"
+	GCSCredType   = "gcs.credtype"
+	GCSUseJsonAPI = "gcs.usejsonapi" // set to anything to enable


Suggested change

GCSUseJsonAPI = "gcs.usejsonapi" // set to anything to enable

GCSUseJSONAPI = "gcs.usejsonapi" // set to anything to enable

ferhatelmas · 2026-02-13T22:46:45Z

io/config.go

+	AdlsSasTokenPrefix         = "adls.sas-token."
+	AdlsConnectionStringPrefix = "adls.connection-string."
+	AdlsSharedKeyAccountName   = "adls.auth.shared-key.account.name"
+	AdlsSharedKeyAccountKey    = "adls.auth.shared-key.account.key"
+	AdlsEndpoint               = "adls.endpoint"
+	AdlsProtocol               = "adls.protocol"


ADLS...

nit: these were here before so feel free to ignore but noting while we're on it

alessandro-nori added 3 commits February 2, 2026 11:10

implement register pattern for io

26fe46b

update LoadFS doc

fc33783

improvements to tests

24273ec

alessandro-nori force-pushed the io_registry_single_module branch from 3ed4082 to 24273ec Compare February 2, 2026 13:04

alessandro-nori changed the title ~~Io registry single module~~ IO Registry Feb 2, 2026

alessandro-nori marked this pull request as ready for review February 2, 2026 13:27

alexandre-normand reviewed Feb 4, 2026

View reviewed changes

zeroshade requested changes Feb 5, 2026

View reviewed changes

alessandro-nori added 3 commits February 6, 2026 09:57

panic if attempting to register the same scheme twice

ec4790b

move io config keys to io package

e18695d

add some tests for mem io

068d8cf

alessandro-nori force-pushed the io_registry_single_module branch from 5411291 to 068d8cf Compare February 6, 2026 11:04

alexandre-normand approved these changes Feb 9, 2026

View reviewed changes

ferhatelmas reviewed Feb 9, 2026

View reviewed changes

run integration tests on all io directory

4556800

github-actions bot added the INFRA label Feb 10, 2026

zeroshade requested changes Feb 10, 2026

View reviewed changes

zeroshade changed the title ~~IO Registry~~ refactor(io): use the registry pattern for IO schemes Feb 10, 2026

use mutex to check for duplicated scheme in registry

c20cd02

ferhatelmas reviewed Feb 12, 2026

View reviewed changes

io/io.go Outdated

}

var (

ErrIONotFound = errors.New("io scheme not registered")

Copy link

Contributor

ferhatelmas Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrIOSchemeNotFound?

ferhatelmas reviewed Feb 12, 2026

View reviewed changes

address review comments

98cd974

ferhatelmas reviewed Feb 13, 2026

View reviewed changes

	GCSUseJsonAPI = "gcs.usejsonapi" // set to anything to enable
	GCSUseJSONAPI = "gcs.usejsonapi" // set to anything to enable

Conversation

alessandro-nori commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Extra notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-nori Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-nori Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alessandro-nori commented Feb 2, 2026 •

edited

Loading

alessandro-nori Feb 4, 2026 •

edited

Loading

alessandro-nori Feb 10, 2026 •

edited

Loading