Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement] Adds the registerIcebergCatalogConfig abstract interface #874

Closed
xunliu opened this issue Nov 29, 2023 · 1 comment · Fixed by #1143
Closed

[Improvement] Adds the registerIcebergCatalogConfig abstract interface #874

xunliu opened this issue Nov 29, 2023 · 1 comment · Fixed by #1143
Assignees
Labels
improvement Improvements on everything

Comments

@xunliu
Copy link
Member

xunliu commented Nov 29, 2023

What would you like to be improved?

The IcebergREST has three impalement IT classes.
I think it better to add the registerIcebergCatalogConfig abstract interface in IcebergRESTServiceBaseIT.class.

How should we improve?

Use an abstract interface replace switch statement.

com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/IcebergRESTServiceBaseIT.java

  private void registerIcebergCatalogConfig() {
    Map<String, String> icebergConfigs;

    switch (catalogType) {
      case HIVE:
        icebergConfigs = getIcebergHiveCatalogConfigs();
        break;
      case JDBC:
        icebergConfigs = getIcebergJdbcCatalogConfigs();
        break;
      case MEMORY:
        icebergConfigs = getIcebergMemoryCatalogConfigs();
        break;
      default:
        throw new RuntimeException("Not support Iceberg catalog type:" + catalogType);
    }

    AbstractIT.registerCustomConfigs(icebergConfigs);
    LOG.info("Iceberg REST service config registered," + StringUtils.join(icebergConfigs));
  }
@FANNG1
Copy link
Contributor

FANNG1 commented Nov 30, 2023

move it to 0.4.0?

jerryshao pushed a commit that referenced this issue Dec 14, 2023
### What changes were proposed in this pull request?
1. make getCatalogConfig abstract, Hive&Jdbc&Memory implement these
interfaces.
2. remove `gravitino-docker-it` tag from IcebergRESTServiceIT.

### Why are the changes needed?
After #711, It takes too much time to test IcebergRESTServiceIT, there's
no need to start the docker container for the memory catalog

Fix: #874 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
existing UT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvements on everything
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants