HADOOP-19425. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part2. #7653
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART2
branch
from
5392250 to
219c529
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART2
branch
from
c8367d7 to
9d2cabf
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART2
branch
2 times, most recently
from
2274249 to
cfdf52f
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART2
branch
from
cfdf52f to
7c44bf5
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
anujmodi2021
left a comment
anujmodi2021
left a comment
There was a problem hiding this comment.
Thanks for all the efforts @slfan1989
I have added a few comments.
| int SCALE_TEST_TIMEOUT_SECONDS = 30 * 60; | ||
|
|
||
| int SCALE_TEST_TIMEOUT_MILLIS = SCALE_TEST_TIMEOUT_SECONDS * 1000; | ||
| int SCALE_TEST_TIMEOUT_MILLIS = SCALE_TEST_TIMEOUT_SECONDS; |
Contributor
There was a problem hiding this comment.
Given that Timeout in Junit5 is supposed to be defined in seconds, this is logically fine, but this does not sound right. May be we should get rid of the redundant variable here or rename it aptly to denote timeout in seconds only.
Contributor
Author
There was a problem hiding this comment.
Thank you very much for your suggestion! I have thought of a solution that has less impact on the code: when introducing Timeout, we can explicitly specify the time unit, so we won't need to modify the variable anymore.
Timeout(value = AzureTestConstants.AZURE_TEST_TIMEOUT, unit = TimeUnit.MILLISECONDS)
| public static final String TEST_CONFIGURATION_FILE_NAME = "azure-test.xml"; | ||
| public static final String TEST_CONTAINER_PREFIX = "abfs-testcontainer-"; | ||
| public static final int TEST_TIMEOUT = 15 * 60 * 1000; | ||
| public static final int TEST_TIMEOUT = 15 * 60; |
Contributor
There was a problem hiding this comment.
Should we rename it as well to TEST_TIMEOUT_IN_SEC ?
Contributor
Author
There was a problem hiding this comment.
As mentioned in the previous feedback, we can avoid modifying this variable.
| import static org.apache.hadoop.fs.azurebfs.utils.UriUtils.changeUrlFromBlobToDfs; | ||
| import static org.apache.hadoop.fs.azurebfs.utils.UriUtils.changeUrlFromDfsToBlob; | ||
| import static org.apache.hadoop.test.LambdaTestUtils.intercept; | ||
| import static org.assertj.core.api.Assertions.assertThat; |
Contributor
There was a problem hiding this comment.
Nitpick: Shouldn't the non-org.apache.hadoop imports be added before the org.apache.hadoop imports?
Contributor
Author
There was a problem hiding this comment.
Your suggestion makes sense, and I have already adjusted the order of the imports.
Contributor
Author
|
@anujmodi2021 I apologize for the slow progress on the Azure module upgrade. The delay was due to waiting for the upgrade of the hadoop-common module. Now that the hadoop-common module upgrade has been completed, I will proceed with the Azure module upgrade as soon as possible. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@anujmodi2021 Could you please help review this pr again? Thank you very much! |
Contributor
|
Thanks @slfan1989 Code changes looks good. Please look and fix them. |
anujmodi2021
requested changes
Jun 30, 2025
Contributor
anujmodi2021
left a comment
anujmodi2021
left a comment
There was a problem hiding this comment.
Test failures to be fixed.
ITestAzureBlobFileSystemMainOperation and ITestAbfsInputStreamSmallFileReads
Contributor
Author
@anujmodi2021 Thank you very much for reviewing the code! I have fixed the related unit tests. Could you kindly check them again? |
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
reviewed
Jul 1, 2025
| Assertions.assertThat(paths.get(1).isDirectory()).isEqualTo(true); | ||
| Assertions.assertThat(paths.get(2).isDirectory()).isEqualTo(true); | ||
| Assertions.assertThat(paths.get(3).isDirectory()).isEqualTo(false); | ||
| Assertions.assertThat(listResultSchema.getNextMarker()).isNotNull(); |
Contributor
Author
There was a problem hiding this comment.
Thank you for pointing out this issue! I will make the necessary corrections.
cnauroth
approved these changes
Jul 1, 2025
Contributor
cnauroth
left a comment
cnauroth
left a comment
There was a problem hiding this comment.
+1 pending pre-submit. Thank you @slfan1989 ! @anujmodi2021 , thank you for reporting the issue.
|
🎊 +1 overall
This message was automatically generated. |
anujmodi2021
approved these changes
Jul 2, 2025
Contributor
anujmodi2021
left a comment
anujmodi2021
left a comment
There was a problem hiding this comment.
Thanks for fixing the tests @slfan1989
+1 LGTM
4 tasks
Contributor
Author
|
@anujmodi2021 @cnauroth @zhtttylz Thank you very much for the review! |
75 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: HADOOP-19425. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part2.
How was this patch tested?
Junit Test & mvn clean test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?