Skip to content

Restore skip archive handler in Iceberg Glue catalog#16564

Merged
ebyhr merged 1 commit intomasterfrom
ebi/iceberg-glue-skip-archive
Mar 16, 2023
Merged

Restore skip archive handler in Iceberg Glue catalog#16564
ebyhr merged 1 commit intomasterfrom
ebi/iceberg-glue-skip-archive

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Mar 15, 2023

Description

Fixes #16561

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Mar 15, 2023
@github-actions github-actions bot added the iceberg Iceberg connector label Mar 15, 2023
@ebyhr ebyhr requested a review from electrum March 15, 2023 22:18
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

buildConfigObject() is intended for conditional configuration of the injector. In this case, we don't need it at binding time, so it's cleaner to use a provider method, just like any other object that we need manually construct based on injected things:

@Provides
@Singleton
@ForGlueHiveMetastore
public static RequestHandler2 createRequestHandler(IcebergGlueCatalogConfig config)
{
    return new SkipArchiveRequestHandler(config.isSkipArchive());
}

While using buildConfigObject() works fine, it feels like a "trick" or "cheat" that doesn't generalize to other situations. If we needed anything else from the injector to construct the instance, it wouldn't work.

ce42133 broke a logic to skip archive in Iceberg Glue catalog.
@ebyhr ebyhr force-pushed the ebi/iceberg-glue-skip-archive branch from b741388 to 209ac9d Compare March 16, 2023 00:32
@ebyhr ebyhr merged commit c98bb40 into master Mar 16, 2023
@ebyhr ebyhr deleted the ebi/iceberg-glue-skip-archive branch March 16, 2023 04:56
@github-actions github-actions bot added this to the 411 milestone Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed iceberg Iceberg connector

Development

Successfully merging this pull request may close these issues.

Fix broken handling for iceberg.glue.skip-archive config property

2 participants