Skip to content

[SPARK-43830][BUILD][FOLLOWUP] Update scalatest and scalatestplus related dependencies to newest version#41364

Closed
panbingkun wants to merge 7 commits intoapache:masterfrom
panbingkun:SPARK-43830
Closed

[SPARK-43830][BUILD][FOLLOWUP] Update scalatest and scalatestplus related dependencies to newest version#41364
panbingkun wants to merge 7 commits intoapache:masterfrom
panbingkun:SPARK-43830

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented May 29, 2023

What changes were proposed in this pull request?

The pr aims to follow up PR: #41341

Why are the changes needed?

Fix issue which caused by inconsistent versions of bytebuddy that selenium and mockito-core rely on.
1.mockito-core depend on bytebuddy 1.12.19
image
https://github.com/mockito/mockito/blob/v4.11.0/gradle/dependencies.gradle#L7

2.selenium depend on bytebuddy 1.14.4
https://github.com/SeleniumHQ/selenium/blob/selenium-4.9.1/java/maven_deps.bzl#L81

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Manual testing:
    (base) panbingkun:~/Developer/spark/spark-community$build/sbt "connect/test"
    ...
    [info] - function_raise_error (2 milliseconds)
    [info] - column_when_otherwise (4 milliseconds)
    [info] - function_date_trunc (4 milliseconds)
    [info] Run completed in 21 seconds, 249 milliseconds.
    [info] Total number of tests run: 576
    [info] Suites: completed 12, aborted 0
    [info] Tests: succeeded 576, failed 0, canceled 0, ignored 0, pending 0
    [info] All tests passed.
    [success] Total time: 77 s (01:17), completed May 29, 2023 4:45:57 PM

  • Pass GA

pom.xml Outdated
<groupId>org.scalatestplus</groupId>
<artifactId>mockito-4-11_${scala.binary.version}</artifactId>
<version>3.2.16.0</version>
<exclusions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these exclusions needed, if we're manually managing the dependency version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's unnecessary, but I think we should copy

<dependency>
      <groupId>net.bytebuddy</groupId>
      <artifactId>byte-buddy</artifactId>
      <scope>test</scope>
    </dependency>
    <dependency>
      <groupId>net.bytebuddy</groupId>
      <artifactId>byte-buddy-agent</artifactId>
      <scope>test</scope>
    </dependency>

to the module using mockito-core because there is just configuring dependencyManagement instead of dependencies

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it feels like one or the other of these changes is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done. @srowen @LuciferYang

@srowen
Copy link
Member

srowen commented May 30, 2023

Wait, do we need a direct dependency? I thought perhaps just managing the dependency would have the desired effect without exclusion. I didn't realize this alternative would change so much. We don't actually directly use this dependnecy.

@panbingkun
Copy link
Contributor Author

panbingkun commented May 30, 2023

eg, When we did not add
image
to resource-managers/yarn/pom.xml
I run ./build/mvn -pl :spark-yarn_2.13 -Phadoop-3 -Phive-2.3 -Pyarn -Phive-thriftserver -Phive dependency:tree
result as follows:
image
From a principle perspective, we need to do this.

@srowen
Copy link
Member

srowen commented May 30, 2023

It appears nowhere in the build, when you do not exclude it, and do not depend on it directly? that seems strange, if it's needed by a third party lib.

@panbingkun
Copy link
Contributor Author

1.When I do not exclude it, and do not depend on it directly, result as follows:
image

2.When I exclude it, and do not depend on it directly, result as follows:
image

@srowen
Copy link
Member

srowen commented May 30, 2023

This is what happens when you set the new version in dependencyManagement, but don't exclude? hm. I'm thinking it's simpler to go back to your first version if so

@panbingkun
Copy link
Contributor Author

When I only add
image

And don't exclude anything, result as follows:
image

It seems to be what we want, and it's very slight.
Let's do it this way!

…plus related dependencies to newest version"

This reverts commit a3071d0.
…plus related dependencies to newest version"

This reverts commit 15b5fe5.
…plus related dependencies to newest version"

This reverts commit eb80676.
@srowen srowen closed this in 8a11008 May 31, 2023
@srowen
Copy link
Member

srowen commented May 31, 2023

Merged to master

@dongjoon-hyun
Copy link
Member

I'm checking the AS-IS master branch. Unfortunately, it seems that I found another instance of mockito-bytebuddy conflicts in YARN module.

$ git log --oneline -n1
8a1100806d (HEAD -> master, apache/master, apache/HEAD) [SPARK-43830][BUILD][FOLLOWUP] Update scalatest and scalatestplus related dependencies to newest version

$ build/sbt -Pyarn "yarn/testOnly *.YarnShuffleServiceMetricsSuite"
...
[info] YarnShuffleServiceMetricsSuite:
[info] org.apache.spark.network.yarn.YarnShuffleServiceMetricsSuite *** ABORTED *** (25 milliseconds)
[info]   java.lang.IllegalStateException: Could not initialize plugin: interface org.mockito.plugins.MockMaker (alternate: null)

Could you double-check the above, @panbingkun ?

@panbingkun
Copy link
Contributor Author

I'm checking the AS-IS master branch. Unfortunately, it seems that I found another instance of mockito-bytebuddy conflicts in YARN module.

$ git log --oneline -n1
8a1100806d (HEAD -> master, apache/master, apache/HEAD) [SPARK-43830][BUILD][FOLLOWUP] Update scalatest and scalatestplus related dependencies to newest version

$ build/sbt -Pyarn "yarn/testOnly *.YarnShuffleServiceMetricsSuite"
...
[info] YarnShuffleServiceMetricsSuite:
[info] org.apache.spark.network.yarn.YarnShuffleServiceMetricsSuite *** ABORTED *** (25 milliseconds)
[info]   java.lang.IllegalStateException: Could not initialize plugin: interface org.mockito.plugins.MockMaker (alternate: null)

Could you double-check the above, @panbingkun ?

Let me check it.

@LuciferYang
Copy link
Contributor

LuciferYang commented May 31, 2023

I think this should be a classpath related issue and only affect sbt test.

May be we should revert this one(I think using byte-buddy 1.12.19 and byte-buddy-agent 1.12.19 should also be ok) and solve the problem by exlcude byte-buddy 1.14.4 in SparkBuild.scala?

@panbingkun
Copy link
Contributor Author

panbingkun commented May 31, 2023

But my local env is ok
image

Can you reproduce it? @LuciferYang

@dongjoon-hyun
Copy link
Member

Oh, let me try in a docker again.

@dongjoon-hyun
Copy link
Member

Thank you for confirming! I also verified that it succeeds cleanly on docker Java images. Sorry for the false alarm.

@dongjoon-hyun
Copy link
Member

$ docker run -it --rm -v $PWD:/spark -w /spark --entrypoint /bin/bash openjdk:17
bash-4.4# build/sbt -Pyarn "yarn/testOnly *.YarnShuffleServiceMetricsSuite"
Using /usr/java/openjdk-17 as default JAVA_HOME.
Note, this will be overridden by -java-home if it is set.
Attempting to fetch sbt
Launching sbt from build/sbt-launch-1.8.3.jar
[info] [launcher] getting org.scala-sbt sbt 1.8.3  (this may take some time)...
[info] [launcher] getting Scala 2.12.17 (for sbt)...
[info] welcome to sbt 1.8.3 (Oracle Corporation Java 17.0.2)
...
[info] YarnShuffleServiceMetricsSuite:
[info] - metrics named as expected (161 milliseconds)
[info] - openBlockRequestLatencyMillis - collector receives correct types (83 milliseconds)
[info] - registerExecutorRequestLatencyMillis - collector receives correct types (3 milliseconds)
[info] - blockTransferRateBytes - collector receives correct types (3 milliseconds)
[info] - blockTransferRate - collector receives correct types (3 milliseconds)
[info] - blockTransferMessageRate - collector receives correct types (2 milliseconds)
[info] - registeredExecutorsSize - collector receives correct types (42 milliseconds)
[info] Run completed in 8 seconds, 699 milliseconds.
[info] Total number of tests run: 7
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 7, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 232 s (03:52), completed May 31, 2023, 3:38:45 AM
bash-4.4#

@LuciferYang
Copy link
Contributor

But my local env is ok image

Can you reproduce it? @LuciferYang

No, It ran successfully in my testing environment

@panbingkun
Copy link
Contributor Author

I think this issue still exists in the SBT compilation, so I will continue to investigate it, finding a better solution.

czxm pushed a commit to czxm/spark that referenced this pull request Jun 12, 2023
…ated dependencies to newest version

### What changes were proposed in this pull request?
The pr aims to follow up PR: apache#41341

### Why are the changes needed?
Fix issue which caused by inconsistent versions of `bytebuddy` that `selenium` and `mockito-core` rely on.
1.mockito-core depend on `bytebuddy` 1.12.19
<img width="491" alt="image" src="https://github.com/apache/spark/assets/15246973/62dbed8c-3164-4337-8d67-a4eb371312bd">
https://github.com/mockito/mockito/blob/v4.11.0/gradle/dependencies.gradle#L7

2.selenium depend on `bytebuddy` 1.14.4
https://github.com/SeleniumHQ/selenium/blob/selenium-4.9.1/java/maven_deps.bzl#L81

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
- Manual testing:
(base) panbingkun:~/Developer/spark/spark-community$build/sbt "connect/test"
...
[info] - function_raise_error (2 milliseconds)
[info] - column_when_otherwise (4 milliseconds)
[info] - function_date_trunc (4 milliseconds)
[info] Run completed in 21 seconds, 249 milliseconds.
[info] Total number of tests run: 576
[info] Suites: completed 12, aborted 0
[info] Tests: succeeded 576, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 77 s (01:17), completed May 29, 2023 4:45:57 PM

- Pass GA

Closes apache#41364 from panbingkun/SPARK-43830.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants