Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jul 30, 2020

What changes were proposed in this pull request?

So far, we fixed many stuffs in core module. This PR fixes the remaining UT failures in Scala 2.13.

  • OneApplicationResource.environmentInfo will return a deterministic result for sparkProperties, hadoopProperties, systemProperties, and classpathEntries.
  • SubmitRestProtocolSuite has Scala 2.13 answer in addition to the existing Scala 2.12 answer, and uses the expected answer based on the Scala runtime version.

Why are the changes needed?

To support Scala 2.13.

Does this PR introduce any user-facing change?

Yes, environmentInfo is changed, but this fixes the indeterministic behavior.

How was this patch tested?

  • Scala 2.12: Pass the Jenkins or GitHub Action
  • Scala 2.13: Do the following.
$ dev/change-scala-version.sh 2.13
$ build/mvn test -pl core --am -Pscala-2.13

BEFORE

Tests: succeeded 2612, failed 3, canceled 1, ignored 8, pending 0
*** 3 TESTS FAILED ***

AFTER

Tests: succeeded 2615, failed 0, canceled 1, ignored 8, pending 0
All tests passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change of this file is a simple sorting.

@dongjoon-hyun
Copy link
Member Author

cc @srowen and @HyukjinKwon
This is the last piece for build/mvn test -pl core --am -Pscala-2.13. After this PR, I'll add the test coverage into GitHub Action for this.

@HyukjinKwon
Copy link
Member

Wow this is nice

@dongjoon-hyun
Copy link
Member Author

Thank you, @HyukjinKwon . BTW, GitHub Action seems to be out of order for some reasons in both master branch and PR builder.

@HyukjinKwon
Copy link
Member

Will take a look.

@dongjoon-hyun
Copy link
Member Author

Thanks. Take your time. This takes some time for review because it needs to run core and the dependencies.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

I was wondering if the order would matter to any user. It shouldn't. That said, if it does, the ordering isn't stable or guaranteed at the moment anyway. So seems OK to me.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 30, 2020

Thanks, @srowen .

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 30, 2020

Rebased to the master to bring @HyukjinKwon 's GitHub Action fixes. Thanks!

@dongjoon-hyun
Copy link
Member Author

All test passed except the ExecutorSuite which is known to be flaky in these day. I'll merge this. Thanks!

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.

3 participants