Skip to content

Comments

[SPARK-39464][CORE][TESTS] Use Utils.localCanonicalHostName instead of localhost in tests#36866

Closed
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-39464
Closed

[SPARK-39464][CORE][TESTS] Use Utils.localCanonicalHostName instead of localhost in tests#36866
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-39464

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jun 14, 2022 •
edited
Loading

What changes were proposed in this pull request?

This PR aims to use Utils.localCanonicalHostName instead of a constant localhost in the following suites.

  • MasterSuite
  • MasterWebUISuite
  • RocksDBBackendHistoryServerSuite

Why are the changes needed?

These test cases fails when we run with SPARK_LOCAL_IP on IPv6-only environment.

Does this PR introduce any user-facing change?

No. This is a test-only change.

How was this patch tested?

Pass the CIs first and manually test on IPv6-only environment.

@dongjoon-hyun dongjoon-hyun mentioned this pull request Jun 14, 2022
Closed
@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 14, 2022

Looks fine. cc @Ngone51 and @jiangxb1987 FYI

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 14, 2022

Thank you, @HyukjinKwon .

LuciferYang
LuciferYang reviewed Jun 14, 2022
View reviewed changes
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala Outdated

private var provider: FsHistoryProvider = null
private var server: HistoryServer = null
private var localhost: String = Utils.localCanonicalHostName()
Copy link
Contributor

@LuciferYang LuciferYang Jun 14, 2022

Choose a reason for hiding this comment

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

localhost can be val

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jun 14, 2022

Choose a reason for hiding this comment

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

Thanks!

Ngone51
Ngone51 reviewed Jun 14, 2022
View reviewed changes
Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

How do we prevent the arbitrary usage of "localhost" in future tests? Would a scalastyle rule help it?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 14, 2022

Thank you, @Ngone51 . In this case, I guess we need to setup IPv6-only CIs eventually.

How do we prevent the arbitrary usage of "localhost" in future tests? Would a scalastyle rule help it?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 14, 2022 •
edited
Loading

Merged to master. Thank you, @HyukjinKwon , @LuciferYang and @Ngone51 .

@dongjoon-hyun dongjoon-hyun deleted the SPARK-39464 branch June 14, 2022 16:00
@LuciferYang LuciferYang mentioned this pull request Jun 15, 2022
Closed
dongjoon-hyun pushed a commit that referenced this pull request Jun 20, 2022
@LuciferYang @dongjoon-hyun
[SPARK-39464][CORE][TESTS][FOLLOWUP] Use Utils.localHostNameForURI in…
540e695 
…stead of Utils.localCanonicalHostName in tests

### What changes were proposed in this pull request?
This PR aims to use `Utils.localHostNameForURI` instead of `Utils.localCanonicalHostName` in the following suites which changed in #36866

- `MasterSuite`
- `MasterWebUISuite`
- `RocksDBBackendHistoryServerSuite`

### Why are the changes needed?
These test cases fails when we run with `SPARK_LOCAL_IP=::1` and `-Djava.net.preferIPv6Addresses=true`

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

### How was this patch tested?

- Pass GA
- Manual test:

1.  `export SPARK_LOCAL_IP=::1`
```
echo $SPARK_LOCAL_IP
::1
```

2. add `-Djava.net.preferIPv6Addresses=true` to MAVEN_OPTS, for example:
```
diff --git a/pom.xml b/pom.xml
index 1ce3b43..3356622 100644
--- a/pom.xml
+++ b/pom.xml
 -2943,7 +2943,7
               <include>**/*Suite.java</include>
             </includes>
             <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
-            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>
+            <argLine>-ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=${CodeCacheSize} ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true -Djava.net.preferIPv6Addresses=true</argLine>
             <environmentVariables>
               <!--
                 Setting SPARK_DIST_CLASSPATH is a simple way to make sure any child processes
```

3. maven test `RocksDBBackendHistoryServerSuite`, `MasterSuite` and `MasterWebUISuite`

```
mvn clean install -DskipTests -pl core -am
mvn clean test -pl core -Dtest=none -DwildcardSuites=org.apache.spark.deploy.history.RocksDBBackendHistoryServerSuite
mvn clean test -pl core -Dtest=none -DwildcardSuites=org.apache.spark.deploy.master.MasterSuite
mvn clean test -pl core -Dtest=none -DwildcardSuites=org.apache.spark.deploy.master.ui.MasterWebUISuite
```

**Before**

RocksDBBackendHistoryServerSuite:

```
- Redirect to the root page when accessed to /history/ *** FAILED ***
  java.net.ConnectException: Connection refused (Connection refused)
  at java.net.PlainSocketImpl.socketConnect(Native Method)
  at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
  at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
  at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
  at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
  at java.net.Socket.connect(Socket.java:613)
  at java.net.Socket.connect(Socket.java:561)
  at sun.net.NetworkClient.doConnect(NetworkClient.java:180)
  at sun.net.www.http.HttpClient.openServer(HttpClient.java:463)
  at sun.net.www.http.HttpClient.openServer(HttpClient.java:558)
  ...
Run completed in 31 seconds, 745 milliseconds.
Total number of tests run: 73
Suites: completed 2, aborted 0
Tests: succeeded 3, failed 70, canceled 0, ignored 0, pending 0
*** 70 TESTS FAILED ***

```

MasterSuite:

```
- master/worker web ui available behind front-end reverseProxy *** FAILED ***
  The code passed to eventually never returned normally. Attempted 487 times over 50.079685917 seconds. Last failure message: Connection refused (Connection refused). (MasterSuite.scala:405)
Run completed in 3 minutes, 48 seconds.
Total number of tests run: 32
Suites: completed 2, aborted 0
Tests: succeeded 29, failed 3, canceled 0, ignored 0, pending 0
*** 3 TESTS FAILED ***
```

MasterWebUISuite:

```
- Kill multiple hosts *** FAILED ***
  java.net.ConnectException: Connection refused (Connection refused)
  at java.net.PlainSocketImpl.socketConnect(Native Method)
  at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
  at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
  at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
  at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
  at java.net.Socket.connect(Socket.java:613)
  at java.net.Socket.connect(Socket.java:561)
  at sun.net.NetworkClient.doConnect(NetworkClient.java:180)
  at sun.net.www.http.HttpClient.openServer(HttpClient.java:463)
  at sun.net.www.http.HttpClient.openServer(HttpClient.java:558)
  ...
Run completed in 7 seconds, 83 milliseconds.
Total number of tests run: 4
Suites: completed 2, aborted 0
Tests: succeeded 0, failed 4, canceled 0, ignored 0, pending 0
*** 4 TESTS FAILED ***

```

**After**

RocksDBBackendHistoryServerSuite:
```
Run completed in 38 seconds, 205 milliseconds.
Total number of tests run: 73
Suites: completed 2, aborted 0
Tests: succeeded 73, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
```

MasterSuite:
```
Run completed in 1 minute, 10 seconds.
Total number of tests run: 32
Suites: completed 2, aborted 0
Tests: succeeded 32, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
```

MasterWebUISuite:
```
Run completed in 6 seconds, 330 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.
```

Closes #36876 from LuciferYang/SPARK-39464-FOLLOWUP.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LuciferYang LuciferYang LuciferYang left review comments

@Ngone51 Ngone51 Ngone51 approved these changes

Assignees

No one assigned

Labels

CORE WEB UI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dongjoon-hyun @HyukjinKwon @LuciferYang @Ngone51