Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Sep 5, 2024

What changes were proposed in this pull request?

The main changes in this pr are as follows:

  1. In resource-managers/kubernetes/core/pom.xml and resource-managers/kubernetes/integration-tests/pom.xml, the build-helper-maven-plugin configuration has been added for the volcano profile to ensure that when the profile is activated with -Pvolcano, the volcano/src/main/scala directory is treated as an additional source path, and volcano/src/test/scala directory is treated as an additional test code path.
  • resource-managers/kubernetes/core/pom.xml
      <build>
        <plugins>
          <plugin>
            <groupId>org.codehaus.mojo</groupId>
            <artifactId>build-helper-maven-plugin</artifactId>
            <executions>
              <execution>
                <id>add-volcano-source</id>
                <phase>generate-sources</phase>
                <goals>
                  <goal>add-source</goal>
                </goals>
                <configuration>
                  <sources>
                    <source>volcano/src/main/scala</source>
                  </sources>
                </configuration>
              </execution>
              <execution>
                <id>add-volcano-test-sources</id>
                <phase>generate-test-sources</phase>
                <goals>
                  <goal>add-test-source</goal>
                </goals>
                <configuration>
                  <sources>
                    <source>volcano/src/test/scala</source>
                  </sources>
                </configuration>
              </execution>
            </executions>
          </plugin>
        </plugins>
      </build>
  • resource-managers/kubernetes/integration-tests/pom.xml
      <build>
        <plugins>
          <plugin>
            <groupId>org.codehaus.mojo</groupId>
            <artifactId>build-helper-maven-plugin</artifactId>
            <executions>
              <execution>
                <id>add-volcano-test-sources</id>
                <phase>generate-test-sources</phase>
                <goals>
                  <goal>add-test-source</goal>
                </goals>
                <configuration>
                  <sources>
                    <source>volcano/src/test/scala</source>
                  </sources>
                </configuration>
              </execution>
            </executions>
          </plugin>
        </plugins>
      </build>
  1. Removed the management configuration for volcano-related source/test code in SPARK-36061 | [SPARK-36061][K8S] Add volcano module and feature step #35422.

Since Spark uses the sbt-pom-reader plugin in its sbt configuration, the behavior of the build-helper-maven-plugin will also propagate to the sbt build process. Therefore, no additional configuration is required in SparkBuild.scala after this pr.

Why are the changes needed?

The previous configuration way was not very friendly to IntelliJ developers: when debugging code in IntelliJ, regardless of whether they were needed or not, the volcano profile had to be activated; otherwise compilation errors would occur when running tests that depended on the kubernetes module's source code, for example org.apache.spark.shuffle.ShuffleChecksumUtilsSuite :

image

Does this PR introduce any user-facing change?

No

How was this patch tested?

  1. Pass GitHub Actions
image image

It can be seen that the test cases VolcanoFeatureStepSuite and VolcanoSuite have been successfully executed.

  1. Manual Testing Using sbt
  • Run build/sbt clean "kubernetes/testOnly *VolcanoFeatureStepSuite" -Pkubernetes, and without -Pvolcano, no tests will be executed:
[info] Passed: Total 0, Failed 0, Errors 0, Passed 0
[info] No tests to run for kubernetes / Test / testOnly
  • Run build/sbt clean "kubernetes/testOnly *VolcanoFeatureStepSuite" -Pkubernetes -Pvolcano, and with -Pvolcano, VolcanoFeatureStepSuite will pass the tests:
[info] VolcanoFeatureStepSuite:
[info] - SPARK-36061: Driver Pod with Volcano PodGroup (74 milliseconds)
[info] - SPARK-36061: Executor Pod with Volcano PodGroup (8 milliseconds)
[info] - SPARK-38455: Support driver podgroup template (224 milliseconds)
[info] - SPARK-38503: return empty for executor pre resource (1 millisecond)
[info] Run completed in 1 second, 268 milliseconds.
[info] Total number of tests run: 4
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 4, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
  • run build/sbt clean "kubernetes/package" -Pkubernetes -Pvolcano, and with -Pvolcano, confirm that spark-kubernetes_2.13-4.0.0-SNAPSHOT.jar contains VolcanoFeatureStep.class

  • run build/sbt clean "kubernetes/package" -Pkubernetes, and without -Pvolcano, confirm that spark-kubernetes_2.13-4.0.0-SNAPSHOT.jar not contains VolcanoFeatureStep.class

  1. Manual Testing Using Maven
  • Run build/mvn clean test -pl resource-managers/kubernetes/core -am -Dtest=none -DwildcardSuites=org.apache.spark.deploy.k8s.features.VolcanoFeatureStepSuite -Pkubernetes, and without -Pvolcano, no tests will be executed:
Discovery starting.
Discovery completed in 80 milliseconds.
Run starting. Expected test count is: 0
DiscoverySuite:
Run completed in 99 milliseconds.
Total number of tests run: 0
Suites: completed 1, aborted 0
Tests: succeeded 0, failed 0, canceled 0, ignored 0, pending 0
No tests were executed.
  • Run build/mvn clean test -pl resource-managers/kubernetes/core -am -Dtest=none -DwildcardSuites=org.apache.spark.deploy.k8s.features.VolcanoFeatureStepSuite -Pkubernetes -Pvolcano, and with -Pvolcano, VolcanoFeatureStepSuite will pass the tests:
Discovery starting.
Discovery completed in 263 milliseconds.
Run starting. Expected test count is: 4
VolcanoFeatureStepSuite:
- SPARK-36061: Driver Pod with Volcano PodGroup
- SPARK-36061: Executor Pod with Volcano PodGroup
- SPARK-38455: Support driver podgroup template
- SPARK-38503: return empty for executor pre resource
Run completed in 624 milliseconds.
Total number of tests run: 4
Suites: completed 2, aborted 0
Tests: succeeded 4, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
  • run build/mvn clean package -pl resource-managers/kubernetes/core -am -DskipTests -Pkubernetes -Pvolcano and with -Pvolcano, confirm that spark-kubernetes_2.13-4.0.0-SNAPSHOT.jar contains VolcanoFeatureStep.class

  • run build/mvn clean package -pl resource-managers/kubernetes/core -am -DskipTests -Pkubernetes and without -Pvolcano, confirm that spark-kubernetes_2.13-4.0.0-SNAPSHOT.jar not contains VolcanoFeatureStep.class

  1. Testing in IntelliJ (both imported as a Maven project and as an sbt project):
  • By default, do not activate volcano, and confirm that volcano-related code is not recognized as source/test code, and does not affect the compilation and testing of other code.
  • Manually activate volcano, and confirm that volcano-related code is recognized as source/test code, and can be compiled and tested normally.
  1. Similar tests were conducted on kubernetes-integration-tests module to confirm the validity of the volcano profile.

Was this patch authored or co-authored using generative AI tooling?

No

@LuciferYang LuciferYang marked this pull request as draft September 5, 2024 05:14
@LuciferYang LuciferYang changed the title Test [K8S][BUILD] Change to using build-helper-maven-plugin to manage the code for volcano Sep 5, 2024
@LuciferYang
Copy link
Contributor Author

Test first

@LuciferYang LuciferYang changed the title [K8S][BUILD] Change to using build-helper-maven-plugin to manage the code for volcano [SPARK-49518][K8S][BUILD] Change to using build-helper-maven-plugin to manage the code for volcano Sep 5, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending CIs). Thank you, @LuciferYang .

@LuciferYang LuciferYang marked this pull request as ready for review September 5, 2024 07:19
@LuciferYang
Copy link
Contributor Author

+1, LGTM (Pending CIs). Thank you, @LuciferYang .

CI Passed, looks ok

@zhengruifeng
Copy link
Contributor

cc @Yikun

@dongjoon-hyun
Copy link
Member

Merged to master. Thank you, @LuciferYang and all!

@LuciferYang
Copy link
Contributor Author

Thanks @dongjoon-hyun ~

@pan3793
Copy link
Member

pan3793 commented Sep 6, 2024

thanks for fixing this, a small suggestion about the folder layout.

instead of

<module>/src/main/scala
<module>/volcano/src/main/scala

how about

<module>/src/main/scala
<module>/src/main/volcano

the former makes volcano like a sub-module but it is not, the latter is also commonly used in the following situations

<module>/src/main/antlr
<module>/src/main/thrift
<module>/src/main/scala
<module>/src/main/scala-2.12
<module>/src/main/scala-2.13

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Sep 6, 2024

@pan3793 Thank you for your suggestion. The current directory structure is based on the structure of the compatibility dir under the hive module.

image
<module>/src/main/scala
<module>/src/main/volcano

For the proposed design mentioned above, should all scala packages and files be placed under the src/main/volcano? And how should we organize the Java code if there will be Java files under the volcano directory in the future?

@LuciferYang LuciferYang deleted the refactor-volcano branch May 2, 2025 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants