You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "kaijchen (via GitHub)" <gi...@apache.org> on 2023/02/08 03:39:57 UTC
[GitHub] [ozone] kaijchen opened a new pull request, #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit 5
kaijchen opened a new pull request, #4257:
URL: https://github.com/apache/ozone/pull/4257
## What changes were proposed in this pull request?
HDDS-7871 (#4236) needs to add a JUnit 5 test to TestKeyManagerImpl.
Better to do the migration first.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7921
## How was this patch tested?
Run test locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4257:
URL: https://github.com/apache/ozone/pull/4257
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit 5
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4257:
URL: https://github.com/apache/ozone/pull/4257#discussion_r1099645582
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -638,8 +633,6 @@ public void testInvalidPrefixAcl() throws IOException {
.build();
// add acl with invalid prefix name
Review Comment:
Hi @adoroszlai @hemantk-12 can you help review this PR?
Especially the following lines in `testInvalidPrefixAcl()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4257:
URL: https://github.com/apache/ozone/pull/4257#discussion_r1099826874
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -638,8 +633,6 @@ public void testInvalidPrefixAcl() throws IOException {
.build();
// add acl with invalid prefix name
- exception.expect(OMException.class);
- exception.expectMessage("Invalid prefix name");
writeClient.addAcl(ozInvalidPrefix, ozAcl1);
Review Comment:
Isn't this (and `setAcl`, `removeAcl`) expected to throw exception? I see the test passes as is, but why? Are these writes no-ops in the test? If so, I think we should remove them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4257:
URL: https://github.com/apache/ozone/pull/4257#discussion_r1135260033
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -638,8 +633,6 @@ public void testInvalidPrefixAcl() throws IOException {
.build();
// add acl with invalid prefix name
- exception.expect(OMException.class);
- exception.expectMessage("Invalid prefix name");
writeClient.addAcl(ozInvalidPrefix, ozAcl1);
Review Comment:
Hi @adoroszlai, it turns out the exception should be thrown here.
There must be some errors on the master branch, see:
https://github.com/apache/ozone/pull/4396/files#r1135258057
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4257:
URL: https://github.com/apache/ozone/pull/4257#discussion_r1099869068
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -638,8 +633,6 @@ public void testInvalidPrefixAcl() throws IOException {
.build();
// add acl with invalid prefix name
- exception.expect(OMException.class);
- exception.expectMessage("Invalid prefix name");
writeClient.addAcl(ozInvalidPrefix, ozAcl1);
Review Comment:
It's calling the real method, but seems only `getAcl()` is validating the prefix.
https://github.com/apache/ozone/blob/11fd5b12fd73b0d47dcf53a91346d842658c56d7/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java#L99-L100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4257:
URL: https://github.com/apache/ozone/pull/4257#issuecomment-1422383942
Thanks @kaijchen for the patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #4257:
URL: https://github.com/apache/ozone/pull/4257#issuecomment-1422385313
Thanks @adoroszlai for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #4257: HDDS-7921. Migrate TestKeyManagerImpl to JUnit5
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4257:
URL: https://github.com/apache/ozone/pull/4257#discussion_r1135271171
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -638,8 +633,6 @@ public void testInvalidPrefixAcl() throws IOException {
.build();
// add acl with invalid prefix name
- exception.expect(OMException.class);
- exception.expectMessage("Invalid prefix name");
writeClient.addAcl(ozInvalidPrefix, ozAcl1);
Review Comment:
> it turns out the exception should be thrown here.
> There must be some errors on the master branch, see:
HDDS-8157 is created to track this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org