-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
HADOOP-19071. Update maven-surefire-plugin from 3.0.0 to 3.2.5. #6537
Conversation
💔 -1 overall
This message was automatically generated. |
(!) A patch to the testing environment has been detected. |
💔 -1 overall
This message was automatically generated. |
@steveloughran Can you help review this PR? Thank you very much! It looks like we can directly upgrade maven-surefire-plugin to 3.2.2. |
Out of curiosity, what was the reason to pick 3.2.2 as the version to upgrade? The latest version seems to be 3.2.5 and there seem to be quite a few fixes between these two versions. |
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.
+1 from me; let's what surprises surface
@sjlee @steveloughran Thank you for your attention to this PR! When I chose this version, I checked the dependencies of the Spark and Flink projects. As of now, Spark is using 3.1.2, and Flink is using 3.2.2, so I opted for 3.2.2. However, we can consider trying 3.2.5. I will proceed to test it immediately. |
(!) A patch to the testing environment has been detected. |
💔 -1 overall
This message was automatically generated. |
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.
+1 for the change on trunk/3.4; let's see what surprises surface...we only need to worry about build time issues, not production ones.
I will submit a pull request to trunk3.4 and hopefully everything goes smoothly. |
@steveloughran Thank you for reviewing the code! I will merge this PR into the trunk branch and continue to progress with the work to support Java 17 compilation. |
…5. (apache#6537) Contributed by Shilun Fan" This reverts commit 555faf2.
…5. (apache#6537) Contributed by Shilun Fan" This reverts commit 555faf2.
Hi @slfan1989 @steveloughran |
This is not good. But looking at the failures I don't know whether to categorise as "test runner regression" or "brittle tests failing under new test runner". Here are some of the ones I've looked at
I think the step here is to move to assertj so asserts fail with meaningful messages, see if the failure can be understood. Ideally you'd want a test which doesn't measure elapsed time, but instead uses counters in the code (here: of throttle events) to assert what took place. Test See this painfully often else where -it means that the protobuf lib was built with a more recent version of java8 than the early oracle ones. Its fixable in your own build (use the older one) or cast ByteBuffer to Buffer. otherwise we need to make sure tests are on a more recent build.
test
not a very meaningful message. suspect that a different ordering of the threads is causing the assert to fail.
Test
this is a timeout during teardown; after this subsequent tests are possibly going to fail. No obvious cause, though again I'd suspect race conditions. Rather than say "hey, let's revert", I'd propose a "surefire update triggers test failures" and see what can be done about addressing them. because we can't stay frozen with surefire versions. |
Some tests are getting skipped as well as I mentioned on the jira TestDirectoryScanner is a known flaky that ain’t related there is already a ticket somewhere |
& some are failing due to protobuf upgrade as part of hadoop thirdparty upgrade, I am checking all the HDFS tests, but little slowly i will create a ticket and link some related ones or so |
Thank you @steveloughran for your response.
I didn't mean to sound rude. Apologies if you felt that way. I understand that when upgrading maven, surefire or protobuf's version, it is very reasonable to see test failures. But in this case, the jenkins build is not running all tests, some of the tests are failing and some of the builds are getting stuck for 20-24 hours and is affecting all the developers. I spent atleast 2-3 days debugging the test failures assuming that this is caused by my patch before posting a message on the slack channel and then @ayushtkn helped me. I am sure other developers have followed the same path. |
@shahrs87 I agree to revert this PR. I've noticed several PRs failing to compile, which is concerning. I will continue to follow up on updating the maven-surefire-plugin; indeed, this requires more testing. |
I have carefully read the comments above. Some unit tests reported errors. We need to re-modify the unit tests. I will actively participate in the modification of this part. |
There is another problem. After this PR, running any unit test using
|
@tasanuma that problem is there even before this, I had a ticket: HADOOP-18701, and was trying to handle that along with several other stuff, now I don't remember why I left it :-) |
@ayushtkn @tasanuma @steveloughran I don't want to roll back this change. I would like to continue pushing for this upgrade. I have noticed several pull requests that seem to compile inaccurately. Nevertheless, I have decided to roll back this change for now to minimize the impact on other members. I'm working on fixing HADOOP-15984 and I'll follow up later with HADOOP-19071. #6567 #6559
|
pity. I do think those failures need to be addressed, as they are less test runner problems than brittle tests which will need to be fixed eventually |
Description of PR
JIRA: HADOOP-19071. Update maven-surefire-plugin from 3.0.0 to 3.2.5.
I upgraded locally and tested the compilation. The compilation result showed success.
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?