[#1724] feat(server):Add server-side REST API support for fileset #1795
Conversation
|
@jerryshao @qqqttt123 I have fixed the pipeline. Please take a review when you have time. Thanks. |
|
Thanks a lot @coolderli for your work. I will take a look when I have time. Sorry we are busy on 0.4.0 code freeze, this feature will be targeted to 0.5.0. |
common/src/main/java/com/datastrato/gravitino/dto/file/FilesetDTO.java
Outdated
Show resolved
Hide resolved
|
Generally LGTM, I need a further round of reviewing. |
|
|
||
| @Nullable | ||
| @JsonProperty("type") | ||
| private Fileset.Type type; |
There was a problem hiding this comment.
If this Is null, we'd better provide a default value. Otherwise, there will be NPE in HadoopCatalog.
There was a problem hiding this comment.
I give a default value in FilesetDTO builder: https://github.com/datastrato/gravitino/pull/1795/files#diff-1d5e20c0e26e4ddafcf9209a1ab3edc7045af6e8e4090ccfd343e6fa20ba5940R87
Is it available?
There was a problem hiding this comment.
I don't think having a default value in FilesetDTO can solve this problem. The variable here will be used for createFileset() method. And we don't have the check here, so it will possibly an issue.
There was a problem hiding this comment.
Sorry for not saying it clearly. IMO, I think it is OK to have null value for type in REST protocol, but we'd better have a default value when using this type.
There was a problem hiding this comment.
yes, I know. I remove the default from the REST protocol to keep the original request for better audit.
common/src/main/java/com/datastrato/gravitino/dto/file/FilesetDTO.java
Outdated
Show resolved
Hide resolved
| dispatcher.createFileset( | ||
| ident, | ||
| request.getComment(), | ||
| request.getType(), |
There was a problem hiding this comment.
Here request.getType() could be null, and it will lead an error when calling dispatcher.createFileset.
common/src/main/java/com/datastrato/gravitino/dto/responses/FilesetResponse.java
Show resolved
Hide resolved
|
Overall LGTM, just left one comment. |
|
@jerryshao @qqqttt123 Could you please review this again? Thanks. |
What changes were proposed in this pull request?
close #1724
Why are the changes needed?
Add server-side REST API support for fileset
Fix: #1724
Does this PR introduce any user-facing change?
(Please list the user-facing changes introduced by your change, including
How was this patch tested?