Trigger scalatest plugin in the integration-test phase #93
Conversation
|
@kimoonkim I ran the build command you listed on a fresh checkout of the k8s-support-alternate-incremental branch (without this change) and didn't see any failures. The exact command was: and the output included Do you have any ideas why that might be? Am I not cleaning intermediate testing artifacts properly? I'd expect this command to fail before your patch and succeed afterwards but what I'm observing is that it's actually succeeding before. |
|
@ash211 Ah, thanks for trying out the commands. (I am also going to do that myself to verify :-)). I noticed your first $ mvn clean command did not specify -Pkubernees-integration-tests, which means it won't remove resource-managers/kubernetes/integration-tests/target dir if the dir exists already. Any chance the target dir was pre-populated? If yes, I think that would explain why it succeeds. Can you also try doing clean and integration-test in a single command. i.e. |
|
FYI, I did try the single |
|
Reproduced the failure also with the two commands above issued on another fresh clone. Please let me know if I should upload the full log. |
|
I found a tool that can display maven build plan. To use the tool, one just needs to add a few lines in ~/.m2/settings.xml: Then, issue Before this change, the intergration-tests project shows the following. Notice scalatest-maven-plugin runs in the test phase, which is before maven-dependency-plugin doing copy-test-spark-jobs in the pre-integration-test phase:
Here is the build plan after this change. Notice there is one more run of scalatest-maven-plugin that triggers in the integration-test phase, which is after the copy-test-spark-jobs. With this change, that's where the KubernetesSuite will run:
|
| See copy-test-spark-jobs execution of maven-dependency-plugin above. --> | ||
| <groupId>org.scalatest</groupId> | ||
| <artifactId>scalatest-maven-plugin</artifactId> | ||
| <configuration>...</configuration> |
There was a problem hiding this comment.
Ah. Turned out they aren't needed. I got this piece from a scalatest user form and the three dots were there just to indicate omission of text.
I am surprised that the presence of three dots in the config did not break maven. Thanks for pushing me to look at this. I'll remove them in the next patch.
There was a problem hiding this comment.
Removed this in the new patch.
| <goal>test</goal> | ||
| </goals> | ||
| <configuration> | ||
| <suffixes>(?<!Suite)</suffixes> |
There was a problem hiding this comment.
I guess the purpose of this negative pattern is to prevent the KubernetesSuite from being tested in the test phase. Better add a comment to explain this?
There was a problem hiding this comment.
Thanks for the suggestion. Added a comment in the new patch.
| <goal>test</goal> | ||
| </goals> | ||
| <configuration> | ||
| <suffixes>(?<=Suite)</suffixes> |
There was a problem hiding this comment.
Do we need explicitly setting this? I don't find it in the scalatest-maven-plugin's section in the top level pom.xml.
There was a problem hiding this comment.
Removed this in the new patch.
|
@kimoonkim good to know the buildplan plugin, very nice tool! |
|
This does seem to make the tests run successfully now though being newer to Maven I'm not fully following why. @mccheah can you please take a look? |
|
@lins05 Thanks for taking a look. Addressed your comments in the new patch. |
|
@ash211 @mccheah Thanks for taking a look. I am also relatively new to Maven myself and I must say Maven is quite complicated. Let me try to explain what I think is happening before this change. Suppose we issue a command like below (which will fail): Maven does three high level things:
For (3), you can find the full list on the linked page, but here are some that are relevant to us: Notice the Now, let's see what the As shown above, those tarballs and jars will be unpacked to the project.build.directory, which will be These are required inputs to The problem is that the scalatest plugin, which will execute KubernetsSuite, triggers only in the test phase by default. (The pom.xml of the resource-managers/kubernetes/integration-tests currently does not specify anything about scalatest plugin. The setting is inherited from the top project pom.xml.) This plugin ordering is displayed well by the build plan plugin in a previous comment. Copying the relevant part here again: So the above KubernetesSuite code will find the target directory missing, leading to the following exception which is raised because the line 50 above referrs to the first item in an empty list causing a NPE: Now, some of us did not encounter this exception before. How is this possible? Here's one sequence of commands that will hide this problem (I have tried this sequence on my local checkout and it does hide the problem).
One more related command that can add to the confusion is doing mvn clean in between (1) and (2), without specifying the two kubernetes profiles. i.e. |
|
+1 this makes sense - the Maven pom was originally created under the assumption that the build process would execute both the |
|
@mccheah Yes, targeting the From the same maven phase list web page above: The downside is that there is a subtle usage issue with this approach. If a user issues a maven command with the test target, then the other modules like With this command, the So when the The failure can be avoided if one issues a maven command specifying the Please let me know what you guys think. |
|
I thought Alternatively, we can try to take a compile-time dependency on |
|
@mccheah How exactly maven reactor handles multi-module dependencies is a bit mysterious to me. I found this blog saying below:
I think we still need to do |
|
Right - the test expects to ship the jar over as the application dependency of the tests. |
|
I also wanted to propose moving the copy-test-spark-jobs action to some phase like What if in the spark-integration-test-jobs's pom, we try to attach the jar:jar goal to the |
|
@lins05 Creating jars at the Also, what do we think the upside of using the |
Emm, what other modules? IIUC the affected modules would only be the |
|
They are what I meant. Add |
|
I see, then that's ok.
Right. |
|
@lins05 @mccheah $ tar tvfz docker-minimal-bundle/target/spark-docker-minimal-bundle_2.11-2.2.0-SNAPSHOT-driver-docker-dist.tar.gz | grep spark-.*.jar | head $ tar tvfz docker-minimal-bundle/target/spark-docker-minimal-bundle_2.11-2.2.0-SNAPSHOT-driver-docker-dist.tar.gz | grep .jar | wc -l |
|
@lins05 Looked at And the pom.xml specifies I can't imagine we build all these jars in non-package phase. That's just too many pom.xml's to touch. If we only make I am afraid this does look like a show-stopper. Thoughts? |
|
Hm, yeah I suppose our findings are showing that the |
|
@mccheah SGTM. Then this PR is ready for merge? |
|
@kimoonkim an enormous thank you for all your work on this PR! Clearly you've put a lot of effort and research into getting this right. I can't say I'm familiar enough with Maven to say this is right, but I think whether it's perfect or not it's certainly a step in the right direction. Let's merge and move closer to running integration tests in Travis (one of the goals coming out of this week's weekly meeting). Thanks again for the well-researched contribution! |
ash211
merged commit 9d250a2
into
apache-spark-on-k8s:k8s-support-alternate-incremental
* Trigger scalatest plugin in the integration-test phase * Clean up unnecessary config section
* Trigger scalatest plugin in the integration-test phase * Clean up unnecessary config section
…on-k8s#93) * Trigger scalatest plugin in the integration-test phase * Clean up unnecessary config section
…on-k8s#93) * Trigger scalatest plugin in the integration-test phase * Clean up unnecessary config section
…tions
### What changes were proposed in this pull request?
In order to avoid frequently changing the value of `spark.sql.adaptive.shuffle.maxNumPostShufflePartitions`, we usually set `spark.sql.adaptive.shuffle.maxNumPostShufflePartitions` much larger than `spark.sql.shuffle.partitions` after enabling adaptive execution, which causes some bucket map join lose efficacy and add more `ShuffleExchange`.
How to reproduce:
```scala
val bucketedTableName = "bucketed_table"
spark.range(10000).write.bucketBy(500, "id").sortBy("id").mode(org.apache.spark.sql.SaveMode.Overwrite).saveAsTable(bucketedTableName)
val bucketedTable = spark.table(bucketedTableName)
val df = spark.range(8)
spark.conf.set("spark.sql.autoBroadcastJoinThreshold", -1)
// Spark 2.4. spark.sql.adaptive.enabled=false
// We set spark.sql.shuffle.partitions <= 500 every time based on our data in this case.
spark.conf.set("spark.sql.shuffle.partitions", 500)
bucketedTable.join(df, "id").explain()
// Since 3.0. We enabled adaptive execution and set spark.sql.adaptive.shuffle.maxNumPostShufflePartitions to a larger values to fit more cases.
spark.conf.set("spark.sql.adaptive.enabled", true)
spark.conf.set("spark.sql.adaptive.shuffle.maxNumPostShufflePartitions", 1000)
bucketedTable.join(df, "id").explain()
```
```
scala> bucketedTable.join(df, "id").explain()
== Physical Plan ==
*(4) Project [id#5L]
+- *(4) SortMergeJoin [id#5L], [id#7L], Inner
:- *(1) Sort [id#5L ASC NULLS FIRST], false, 0
: +- *(1) Project [id#5L]
: +- *(1) Filter isnotnull(id#5L)
: +- *(1) ColumnarToRow
: +- FileScan parquet default.bucketed_table[id#5L] Batched: true, DataFilters: [isnotnull(id#5L)], Format: Parquet, Location: InMemoryFileIndex[file:/root/opensource/apache-spark/spark-3.0.0-SNAPSHOT-bin-3.2.0/spark-warehou..., PartitionFilters: [], PushedFilters: [IsNotNull(id)], ReadSchema: struct<id:bigint>, SelectedBucketsCount: 500 out of 500
+- *(3) Sort [id#7L ASC NULLS FIRST], false, 0
+- Exchange hashpartitioning(id#7L, 500), true, [id=apache-spark-on-k8s#49]
+- *(2) Range (0, 8, step=1, splits=16)
```
vs
```
scala> bucketedTable.join(df, "id").explain()
== Physical Plan ==
AdaptiveSparkPlan(isFinalPlan=false)
+- Project [id#5L]
+- SortMergeJoin [id#5L], [id#7L], Inner
:- Sort [id#5L ASC NULLS FIRST], false, 0
: +- Exchange hashpartitioning(id#5L, 1000), true, [id=apache-spark-on-k8s#93]
: +- Project [id#5L]
: +- Filter isnotnull(id#5L)
: +- FileScan parquet default.bucketed_table[id#5L] Batched: true, DataFilters: [isnotnull(id#5L)], Format: Parquet, Location: InMemoryFileIndex[file:/root/opensource/apache-spark/spark-3.0.0-SNAPSHOT-bin-3.2.0/spark-warehou..., PartitionFilters: [], PushedFilters: [IsNotNull(id)], ReadSchema: struct<id:bigint>, SelectedBucketsCount: 500 out of 500
+- Sort [id#7L ASC NULLS FIRST], false, 0
+- Exchange hashpartitioning(id#7L, 1000), true, [id=apache-spark-on-k8s#92]
+- Range (0, 8, step=1, splits=16)
```
This PR makes read bucketed tables always obeys `spark.sql.shuffle.partitions` even enabling adaptive execution and set `spark.sql.adaptive.shuffle.maxNumPostShufflePartitions` to avoid add more `ShuffleExchange`.
### Why are the changes needed?
Do not degrade performance after enabling adaptive execution.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Unit test.
Closes apache#26409 from wangyum/SPARK-29655.
Authored-by: Yuming Wang <yumwang@ebay.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@ash211 @cvpatel @ssuchter
What changes were proposed in this pull request?
Currently, the kubernetes integration test runs in the maven test phase and fails because the test jobs and other jars are missing in the target dir. (See #74) Those jars are supposed to be copied in the pre-integration-test phase, which is after the test phase.
This change fixes the issue by triggering the scalatest plugin in the integration-test phase. The target directory now has the needed jars.
How was this patch tested?
Ran the integration test build command and saw it passed the previous failing point.