-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-17751. [ARR] Add unit tests using asynchronous router rpc for all in org.apache.hadoop.hdfs.server.federation.router. #7470
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
base: trunk
Are you sure you want to change the base?
Conversation
@hfutatzhanghb Thank you for your contribution! However, I don't think adding unit tests for these packages is the best approach. If it's just a change in configuration, using parameterized tests would be a more appropriate choice, as it allows for maximum reuse of the existing test code. We can refer to the following documentation for more details: JUnit 5 Parameterized Tests. cc: @ayushtkn @steveloughran @Hexiaoqiao We can refer to
|
Sir, Thanks a lot for your valuable suggestions. Will use parameterized tests to implement this.
…---- Replied Message ----
| From | ***@***.***> |
| Date | 03/05/2025 21:44 |
| To | ***@***.***> |
| Cc | ***@***.***>***@***.***> |
| Subject | Re: [apache/hadoop] HDFS-17751. [ARR] Add unit tests using asynchronous router rpc for all in org.apache.hadoop.hdfs.server.federation.router. (PR #7470) |
@hfutatzhanghb Thank you for your contribution! However, I don't think adding unit tests for these packages is the best approach. If it's just a change in configuration, using parameterized tests would be a more appropriate choice, as it allows for maximum reuse of the existing test code. We can refer to the following documentation for more details: JUnit 5 Parameterized Tests.
cc: @***@***.***@Hexiaoqiao
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
slfan1989 left a comment (apache/hadoop#7470)
@hfutatzhanghb Thank you for your contribution! However, I don't think adding unit tests for these packages is the best approach. If it's just a change in configuration, using parameterized tests would be a more appropriate choice, as it allows for maximum reuse of the existing test code. We can refer to the following documentation for more details: JUnit 5 Parameterized Tests.
cc: @***@***.***@Hexiaoqiao
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
💔 -1 overall
This message was automatically generated. |
@hfutatzhanghb Thank you very much for being willing to add unit tests, as it will significantly improve the robustness of the system. However, we need to have a deeper discussion regarding these changes to the unit tests. Recently, I’ve been upgrading to JUnit 5, so I’m particularly focused on testing-related topics. I’ve initially reviewed the relevant tests and found that the main difference between the synchronous and asynchronous routers lies in the configuration within the My initial suggestion is to use JUnit 5’s Parameterized Tests so that we can validate both configurations in the same test class. However, after discussing with hfutatzhanghb, I learned an important point: HDFS Router tests typically require initializing a minicluster, and miniclusters are resource-intensive components. If we use Parameterized Tests, it would mean starting a minicluster for each test case, which would incur a significant overhead. I have considered two alternative approaches for the current situation: Option 1: When using Parameterized Tests, we can use the After the test completes, we would need to destroy the corresponding MiniCluster to release the resources. Here is an example code:
Option 2: Referencing Before each test method is executed, we can check whether the synchronous or asynchronous cluster has been created. If not, we create the cluster and store the information in Here is a part of the code:
|
@slfan1989 Sir, thanks so much for providing the approach to solve this problem gracefully. I will dive into these two suggestions and push this pr soonly. @Hexiaoqiao @KeeProMise Sir, please also cc. Thanks all again. |
@hfutatzhanghb I have no objections to other PRs, but if you need to make large-scale modifications to the unit tests to support ARR, feel free to @ me, and we can discuss it further. We aim to improve the system through unit tests, but we also want to adopt a better approach, rather than just relying on class inheritance. Thanks again for your contribution! |
Sir, totally agree with you, we should make system robust and code clean in the meantime. Thanks for your help. |
713d92f
to
099e03c
Compare
💔 -1 overall
This message was automatically generated. |
hi @hfutatzhanghb I think other PRs are not closely related to UT. They are meant to solve other issues, so they shouldn't be blocked. |
Sir, thanks for responding. Got it. |
b8e48d3
to
82b67db
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
f1ae3aa
to
afeab77
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
afeab77
to
0a1c567
Compare
💔 -1 overall
This message was automatically generated. |
or create two long-lived HDFS mini clusters with the configs, and have the test cases choose which to use based on the parameters |
🎊 +1 overall
This message was automatically generated. |
@steveloughran Sir, thanks for your advice. I will give an example soonly and then let's choose one way. |
@hfutatzhanghb Thank you very much for your contribution! From my personal perspective, the code in this version meets expectations. However, we still need to gather feedback from other members. I will provide more specific suggestions later this week. |
if @slfan1989 is happy, so am I. I don't know hdfs well enough to review things there, at least not enough to merge things |
Thanks @slfan1989 and @steveloughran , i will start develope this PR after HDFS-17749 merged~ |
Hi, @slfan1989 @KeeProMise @Hexiaoqiao . I wonder that whether should we split this PR or not ? If all in one, it would be unfriendly for reivewing. What's your opinions? |
@hfutatzhanghb Thank you for the information! I think we should first focus on updating all the unit tests in a single pr, and then consider splitting the changes into multiple prs afterward. I believe a single PR should not exceed 3000 lines or affect more than 50 classes. Since we're only improving unit tests and not modifying core logic, the review effort should be manageable. |
Got it, Thanks again! |
+1 |
0a1c567
to
bd02510
Compare
💔 -1 overall
This message was automatically generated. |
bd02510
to
a1a6411
Compare
💔 -1 overall
This message was automatically generated. |
a1a6411
to
0353873
Compare
💔 -1 overall
This message was automatically generated. |
10cbb16
to
6f9b966
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
6f9b966
to
ea09279
Compare
💔 -1 overall
This message was automatically generated. |
ea09279
to
8e72842
Compare
💔 -1 overall
This message was automatically generated. |
…l in org.apache.hadoop.hdfs.server.federation.router.
8e72842
to
53a5ee7
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Description of PR
All unit tests in
org.apache.hadoop.hdfs.server.federation.router
package as below:I will add async version soonly.
❎ means no needs to modify
✅ means done
How was this patch tested?
Add unit tests.