-
Notifications
You must be signed in to change notification settings - Fork 319
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
[#2041][#2306] Improvement(catalog-hadoop):Add E2E integration test for Hadoop catalog #2294
Conversation
50ba9a4
to
39ad1d7
Compare
@jerryshao @yuqi1129 @qqqttt123 @mchades Could you help me review this? Thanks. |
@@ -575,7 +578,7 @@ private FilesetEntity updateFilesetEntity( | |||
.withName(newName) | |||
.withNamespace(ident.namespace()) | |||
.withId(filesetEntity.id()) | |||
.withComment(filesetEntity.comment()) | |||
.withComment(newComment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a UT to cover this?
break; | ||
case TABLE: | ||
case FILESET: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, I think it is necessary to have UTs to cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuqi1129 can you please help to review this part and the UT added below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jerryshao
He has added test cases about it, please see validateDeleteSchemaCascade
, validateDeleteCatalogCascade
, and validateDeleteFilesetCascade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other parts are fine for me.
336c3aa
to
b0f1d60
Compare
@jerryshao Could you take a look, I refactor some UTs. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot @coolderli for your work.
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #2041
Fix: #2306
Does this PR introduce any user-facing change?
How was this patch tested?
./gradlew build