You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "xiongbo-sjtu (via GitHub)" <gi...@apache.org> on 2023/10/01 05:31:31 UTC
[GitHub] [spark] xiongbo-sjtu opened a new pull request, #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
xiongbo-sjtu opened a new pull request, #43194:
URL: https://github.com/apache/spark/pull/43194
### Why are the changes needed?
It's discovered from [Github Actions](https://github.com/xiongbo-sjtu/spark/actions/runs/6270601155/job/17028788767) that StatusTrackerSuite can run into random failures because FutureAction.jobIds is not a sorted sequence (by design), as shown in the following stack trace (highlighted in red). The proposed fix is to update the unit test to remove the nondeterministic behavior.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
```
build/mvn package -DskipTests -pl core
build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.StatusTrackerSuite test
```
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742103845
> as shown in the following stack trace (highlighted in red)
which stack? Seems some information is missing in the PR description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1343460096
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -140,6 +140,14 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
}
sc.removeJobTag("tag1")
+ eventually(timeout(60.seconds)) {
+ // Make sure that only two jobs are tagged after tag1
+ firstJobFuture.isCompleted should be(true)
+ secondJobFuture.isCompleted should be(true)
+ sc.getJobTags should be (Set("tag2"))
+ sc.statusTracker.getJobIdsForTag("tag1").toSet.size should be(2)
+ }
+
Review Comment:
This is not going to fix the issue - I have detailed the fix required for address the flakiness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1744142296
@xiongbo-sjtu to clarify, when I mentioned about two jobs, I was not referring to `firstJobFuture` and `secondJobFuture` - but two jobs submitted as part of `takeAsync's` implementation when `thirdJobFuture` executes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1745910332
Merged to master.
Thanks for fixing this @xiongbo-sjtu !
Thanks for the review @dongjoon-hyun, @LuciferYang :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1342148839
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -33,7 +33,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
val jobId: Int = eventually(timeout(10.seconds)) {
val jobIds = jobFuture.jobIds
jobIds.size should be(1)
- jobIds.head
+ jobIds.sorted.reverse.head
Review Comment:
If the goal is to get the maximum value in JobIds, can we use `jobIds.max`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xiongbo-sjtu commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742177918
@dongjoon-hyun I've updated the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1744316013
> when I mentioned about two jobs, I was not referring to `firstJobFuture` and `secondJobFuture` - but two jobs submitted as part of `takeAsync's` implementation when `thirdJobFuture` executes.
Make sense. Updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xiongbo-sjtu commented on a diff in pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1342178612
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -33,7 +33,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
val jobId: Int = eventually(timeout(10.seconds)) {
val jobIds = jobFuture.jobIds
jobIds.size should be(1)
- jobIds.head
+ jobIds.sorted.reverse.head
Review Comment:
Good point. Updated the code in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742016816
Thank you for making a PR, @xiongbo-sjtu .
It seems that something is missing in your PR description. Where is the `red` part? Maybe, you didn't copy it from Apache Spark JIRA to here?
> because FutureAction.jobIds is not a sorted sequence (by design), as shown in the following stack trace (highlighted in red).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1765275406
> @xiongbo-sjtu Would it be convenient to submit a similar patch for branch-3.5? I found that this case would also likely fail in branch-3.5.
>
> https://github.com/apache/spark/actions/runs/6510937376/job/17685699603
Here we go.
https://github.com/apache/spark/pull/43388
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1342148839
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -33,7 +33,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
val jobId: Int = eventually(timeout(10.seconds)) {
val jobIds = jobFuture.jobIds
jobIds.size should be(1)
- jobIds.head
+ jobIds.sorted.reverse.head
Review Comment:
If the goal is to get the largest JobId, can we use `jobIds.max`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
URL: https://github.com/apache/spark/pull/43194
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1763312763
@xiongbo-sjtu Would it be convenient to submit a similar patch for branch-3.5? I found that this case would also likely fail in branch-3.5.
https://github.com/apache/spark/actions/runs/6510937376/job/17685699603
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742205990
So looking at it more, the race condition is due to the following:
* We have two jobs for the `takeAsync(999)` - but they are executed one after another.
* In most cases, the ` val thirdJobId = eventually(timeout(10.seconds)) {` will detect the first job start and return that value.
* Subsequently, the last eventually starts and it will immediately match `sc.statusTracker.getJobIdsForTag("tag2").toSet should be (Set(secondJobId, thirdJobId))` - since the second job for `takeAsync` has not yet started.
In some corner cases, the second job for `takeAsync` would have started before the last `eventually` - and so now we have 3 jobs tagged with `tag2` instead of 2.
To fix this, IMO we should do the following:
```
val thirdJobIds = eventually(timeout(10.seconds)) {
if (thirdJobFuture.jobIds.size != 2) throw new IllegalStateException("Both jobs not yet started")
thirdJobFuture.jobIds
}
eventually(timeout(10.seconds)) {
sc.statusTracker.getJobIdsForTag("tag1").toSet should be (
Set(firstJobId, secondJobId))
sc.statusTracker.getJobIdsForTag("tag2").toSet should be (
Set(secondJobId) ++ , thirdJobIds)
}
```
(Paraphrasing - some such).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xiongbo-sjtu commented on a diff in pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1342178612
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -33,7 +33,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
val jobId: Int = eventually(timeout(10.seconds)) {
val jobIds = jobFuture.jobIds
jobIds.size should be(1)
- jobIds.head
+ jobIds.sorted.reverse.head
Review Comment:
Good point. Updated the code in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43194:
URL: https://github.com/apache/spark/pull/43194#discussion_r1342182541
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -33,7 +33,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
val jobId: Int = eventually(timeout(10.seconds)) {
val jobIds = jobFuture.jobIds
jobIds.size should be(1)
- jobIds.head
+ jobIds.max
Review Comment:
Revert ? `jobIds.size` is 1, so `head` == `max`
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -121,7 +121,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
// countAsync()
val firstJobFuture = sc.parallelize(1 to 1000).countAsync()
val firstJobId = eventually(timeout(10.seconds)) {
- firstJobFuture.jobIds.head
+ firstJobFuture.jobIds.max
}
Review Comment:
Revert - same as above.
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -105,7 +105,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
sc.statusTracker.getJobIdsForGroup("my-job-group2") shouldBe empty
val firstJobFuture = sc.parallelize(1 to 1000, 2).takeAsync(999)
eventually(timeout(10.seconds)) {
- firstJobFuture.jobIds.head
+ firstJobFuture.jobIds.max
}
Review Comment:
Revert, same as above.
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -61,7 +61,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
// Passing `null` should return jobs that were not run in a job group:
val defaultJobGroupFuture = sc.parallelize(1 to 1000).countAsync()
val defaultJobGroupJobId = eventually(timeout(10.seconds)) {
- defaultJobGroupFuture.jobIds.head
+ defaultJobGroupFuture.jobIds.max
}
Review Comment:
Revert ? There should only be a single job submitted here, so `head` will be `max`
##########
core/src/test/scala/org/apache/spark/StatusTrackerSuite.scala:
##########
@@ -92,7 +92,7 @@ class StatusTrackerSuite extends SparkFunSuite with Matchers with LocalSparkCont
sc.statusTracker.getJobIdsForGroup("my-job-group2") shouldBe empty
val firstJobFuture = sc.parallelize(1 to 1000, 1).takeAsync(1)
val firstJobId = eventually(timeout(10.seconds)) {
- firstJobFuture.jobIds.head
+ firstJobFuture.jobIds.max
}
Review Comment:
We can revert this as well - same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742194420
Looking into this more, I dont think this PR will fix the issue being seen.
The exception indicates this is not a sorting issue - it is two `Set's` being compared.
The number of entries is incorrect - 2 jobs were expected for each of the two tags, but 3 were found.
We will have to find out what the race condition here is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xiongbo-sjtu commented on pull request #43194: [SPARK-45283][CORE][TESTS] Make StatusTrackerSuite less fragile
Posted by "xiongbo-sjtu (via GitHub)" <gi...@apache.org>.
xiongbo-sjtu commented on PR #43194:
URL: https://github.com/apache/spark/pull/43194#issuecomment-1742343900
I've updated the PR to ensure that the following conditions are met, before `thirdJobFuture` is even constructed.
- [Condition A] firstJobFuture.isCompleted should be(true)
- [Condition B] secondJobFuture.isCompleted should be(true)
- [Condition C] sc.getJobTags.size should be(1)
- [Condition D] sc.statusTracker.getJobIdsForTag("tag1").toSet.size should be(2)
Conditions A & B tell us that the first 2 future actions are completed (i.e., no more state transition).
Condition C means that only 1 tag remains (i.e., `tag1` is removed and `tag2` is kept).
Condition D means that only 2 jobs have been marked with `tag1`.
Given that, we'd expect that the reported issue disappears in the sense that `sc.statusTracker.getJobIdsForTag("tag1").toSet` won't return a set of 3 items.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org