[#5438] feat(iceberg): add view event for Iceberg REST server #5584
Conversation
...st-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java
Outdated
Show resolved
Hide resolved
| @DeveloperApi | ||
| public class IcebergCreateViewEvent extends IcebergViewEvent { | ||
|
|
||
| private CreateViewRequest createViewRequest; |
There was a problem hiding this comment.
Just noticed IcebergTableEvent doesn't add final, do you like to add final to table event too?
There was a problem hiding this comment.
Sure, you mean that createRequest should be final?
There was a problem hiding this comment.
yes, because it will not be changed after initiated, could you change all class members to final for the Iceberg Event?
There was a problem hiding this comment.
I have added final to the Iceberg view event, and I plan to open another PR to add final to the table event and fix the return status code. I think making these changes in a separate PR is better, as this one would otherwise involve modifying too many files.
Please let me know if you're okay with this approach.
...st-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergViewFailureEvent.java
Outdated
Show resolved
Hide resolved
| renameViewRequest.destination()); | ||
| IcebergRequestContext context = new IcebergRequestContext(httpServletRequest(), catalogName); | ||
| viewOperationDispatcher.renameView(context, renameViewRequest); | ||
| return IcebergRestUtils.okWithoutContent(); |
There was a problem hiding this comment.
According to https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L1737, we should return 204 not 200.
There was a problem hiding this comment.
Ok, seems like exists should also return 204
There was a problem hiding this comment.
Yes, thanks for pointing this ! Do you like to propose a PR to fix it, table exists and schema exists should be corrected too.
There was a problem hiding this comment.
Iceberg schema is handled in IcebergNamespaceOperations
|
@orenccl thanks for your work, LGTM except minor comments |
|
@orenccl , merged to main, thanks for your work and figuring out the hidden bugs! |
What changes were proposed in this pull request?
Add view event for Iceberg REST server
Why are the changes needed?
Close: #5438
Does this PR introduce any user-facing change?
No
How was this patch tested?
unit test