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.

Sign up for GitHub

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

Jump to bottom

[#287] feat(iceberg): add iceberg rest catalog namespace interface #302

Merged
merged 1 commit into from
Sep 6, 2023
Merged

[#287] feat(iceberg): add iceberg rest catalog namespace interface #302

merged 1 commit into from
Sep 6, 2023

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Aug 30, 2023 •
edited by jerryshao
Loading

What changes were proposed in this pull request?

to support iceberg rest api, we add interfaces:

  1. config
  2. create&drop&get&list schema

Why are the changes needed?

iceberg api spec

Fix: #287

Does this PR introduce any user-facing change?

add new interfaces

How was this patch tested?

  1. manually tested in local env with memory catalog
  2. add ut to test rest interfaces

@FANNG1 FANNG1 marked this pull request as draft August 30, 2023 10:13
@FANNG1
Copy link
Contributor Author

FANNG1 commented Aug 31, 2023

@jerryshao , UT is added, please take a review while you have time

@FANNG1 FANNG1 marked this pull request as ready for review August 31, 2023 08:02
@github-actions
Copy link

github-actions bot commented Sep 1, 2023 •
edited
Loading

Code Coverage Report

Overall Project 63.42% -0.03% 🟢
Files changed 97.83% 🟢

Module Coverage
catalog-lakehouse 97.83% -2.17% 🟢
Files
Module File Coverage
catalog-lakehouse IcebergExceptionMapper.java 100% 🟢
IcebergRestUtils.java 100% 🟢
IcebergObjectMapperProvider.java 100% 🟢
IcebergConfig.java 100% 🟢
IcebergNamespaceOperations.java 100% 🟢
IcebergTableOps.java 94.51% -5.49% 🟢
IcebergCatalogUtil.java 93.02% -6.98% 🟢

@justinmclean
Copy link
Member

Are we OK with the -52.24% test coverage in IcebergCatalogUtil.java?

@jerryshao
Copy link
Contributor

I would suggest to break into small PRs for iceberg support.

jerryshao
jerryshao reviewed Sep 1, 2023
View reviewed changes
settings.gradle.kts Outdated Show resolved Hide resolved
@jerryshao
Copy link
Contributor

I'm thinking that we should not do a specific catalog only for "iceberg", at least from package organization and naming thing, we should use lakehouse instead of iceberg.

@FANNG1
Copy link
Contributor Author

FANNG1 commented Sep 4, 2023

Are we OK with the -52.24% test coverage in IcebergCatalogUtil.java?

add more tests for IcebergCatalogUtil.java

@FANNG1 FANNG1 mentioned this pull request Sep 5, 2023
Closed
14 tasks
jerryshao
jerryshao reviewed Sep 5, 2023
View reviewed changes
...g-lakehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/ops/IcebergTableOps.java Outdated Show resolved Hide resolved
...g-lakehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/IcebergServerConfig.java Outdated Show resolved Hide resolved
...g-lakehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/ops/IcebergTableOps.java Outdated
if (catalog instanceof SupportsNamespaces) {
asNamespaceCatalog = (SupportsNamespaces) catalog;
LOG.info("init iceberg namespace ops");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the behavior if this Catalog doesn't support SupportsNamespaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now IcebergExceptionMapper catches the NullPointerException and response with http status 500, we could throw 'NotSupportedException' and return http status 406

Copy link
Contributor

Choose a reason for hiding this comment

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

NPE is not an exception that should be caught, it is the bug need to be fixed. So you need to add such logic to handle this thing.

...ehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/utils/IcebergCatalogUtil.java Outdated Show resolved Hide resolved
...-lakehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/web/IcebergRestUtils.java Outdated
return Response.status(Response.Status.OK).entity(t).type(MediaType.APPLICATION_JSON).build();
}

public static Response errorResponse(Exception exc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use "exc" not "ex" as a short name of exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not fixed.

...akehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/web/rest/IcebergConfig.java Outdated Show resolved Hide resolved
...rc/main/java/com/datastrato/graviton/lakehouse/iceberg/web/rest/IcebergSchemaOperations.java Outdated Show resolved Hide resolved
...rc/main/java/com/datastrato/graviton/lakehouse/iceberg/web/rest/IcebergSchemaOperations.java Outdated Show resolved Hide resolved
...lakehouse/src/main/java/com/datastrato/graviton/lakehouse/iceberg/web/rest/IcebergToken.java Outdated Show resolved Hide resolved
server/src/main/java/com/datastrato/graviton/server/GravitonServer.java Outdated Show resolved Hide resolved
@FANNG1 FANNG1 marked this pull request as draft September 5, 2023 15:11
@FANNG1 FANNG1 marked this pull request as ready for review September 6, 2023 02:44
@FANNG1
Copy link
Contributor Author

FANNG1 commented Sep 6, 2023

@jerryshao , the code is updated.

@jerryshao jerryshao changed the title [#286] feat(iceberg) add iceberg rest catlog config, token, schema interfaces [#286] feat(iceberg): add iceberg rest catalog namespace interface Sep 6, 2023
@jerryshao
Copy link
Contributor

You actually don't implement config and token interface, you just bypass them, so in your PR description, you mentioned that this PR fixes #286 , this is not correct. All you addressed is just #287 . Only when you fully support config and token interface, you could solve #286

@FANNG1 FANNG1 marked this pull request as draft September 6, 2023 12:13
@jerryshao jerryshao changed the title [#286] feat(iceberg): add iceberg rest catalog namespace interface [#287] feat(iceberg): add iceberg rest catalog namespace interface Sep 6, 2023
@jerryshao jerryshao marked this pull request as ready for review September 6, 2023 12:35
@jerryshao jerryshao merged commit f95d6ae into apache:main Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi1129 yuqi1129 yuqi1129 left review comments

@jerryshao jerryshao jerryshao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] support namespace ops for iceberg rest server
4 participants
@FANNG1 @justinmclean @jerryshao @yuqi1129