You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "holdenk (via GitHub)" <gi...@apache.org> on 2023/01/25 00:40:24 UTC
[GitHub] [spark] holdenk opened a new pull request, #39728: [SPARK-42173][CORE] RpcAddress equality can fail
holdenk opened a new pull request, #39728:
URL: https://github.com/apache/spark/pull/39728
### What changes were proposed in this pull request?
When constructing an RpcAddress use InetUtils to get a consistently formatted IPv6 address if the env is for an IPv6 address.
### Why are the changes needed?
We use RpcAddress equality for various tasks involving executors and a mismatch of equality can cause interesting errors.
### Does this PR introduce _any_ user-facing change?
Log messages might change from sometimes having all the 0s in a v6 address present to not.
### How was this patch tested?
Existing tests + new unit test showing that [::0:1] is formatted to [::1]
--
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 a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1086103013
##########
core/src/main/scala/org/apache/spark/rpc/RpcAddress.scala:
##########
@@ -23,30 +23,37 @@ import org.apache.spark.util.Utils
/**
* Address for an RPC environment, with hostname and port.
*/
-private[spark] case class RpcAddress(_host: String, port: Int) {
-
- lazy val host: String = Utils.addBracketsIfNeeded(_host)
+private[spark] case class RpcAddress(host: String, port: Int) {
def hostPort: String = host + ":" + port
/** Returns a string in the form of "spark://host:port". */
def toSparkURL: String = "spark://" + hostPort
override def toString: String = hostPort
+
+
Review Comment:
Could you revert this empty line addition?
--
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 closed pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
URL: https://github.com/apache/spark/pull/39728
--
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 #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39728:
URL: https://github.com/apache/spark/pull/39728#issuecomment-1404644646
Could you reply on @attilapiros 's comment, @holdenk ?
--
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] holdenk commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1087233459
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1109,6 +1109,24 @@ private[spark] object Utils extends Logging {
}
}
+ /**
+ * Normalize IPv6 IPs and no-op on all other hosts.
+ */
+ private[spark] def normalizeIpIfNeeded(host: String): String = {
+ // Is this a v6 address. We ask users to add [] around v6 addresses as strs but
+ // there not always there. If it's just 0-9 and : and [] we treat it as a v6 address.
+ // This means some invalid addresses are treated as v6 addresses, but since they are
+ // not valid hostnames it doesn't matter.
+ // See https://www.rfc-editor.org/rfc/rfc1123#page-13 for context around valid hostnames.
+ val addressRe = """^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""".r
Review Comment:
Ah good point, I'm used to v4 addresses where that would be a reserved broadcast address but yeah.
--
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] attilapiros commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "attilapiros (via GitHub)" <gi...@apache.org>.
attilapiros commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1087515031
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1118,7 +1118,7 @@ private[spark] object Utils extends Logging {
// This means some invalid addresses are treated as v6 addresses, but since they are
// not valid hostnames it doesn't matter.
// See https://www.rfc-editor.org/rfc/rfc1123#page-13 for context around valid hostnames.
- val addressRe = """^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""".r
+ val addressRe = """^\[{0,1}([0-9:]+?:[0-9]*)\]{0,1}$""".r
Review Comment:
Absolutely true!
--
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] attilapiros commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "attilapiros (via GitHub)" <gi...@apache.org>.
attilapiros commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1087250704
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1118,7 +1118,7 @@ private[spark] object Utils extends Logging {
// This means some invalid addresses are treated as v6 addresses, but since they are
// not valid hostnames it doesn't matter.
// See https://www.rfc-editor.org/rfc/rfc1123#page-13 for context around valid hostnames.
- val addressRe = """^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""".r
+ val addressRe = """^\[{0,1}([0-9:]+?:[0-9]*)\]{0,1}$""".r
Review Comment:
Is there any reason why not use just this tiny regexp:
```
"""^\[{0,1}([0-9:]+)\]{0,1}$"""
```
--
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] holdenk commented on pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #39728:
URL: https://github.com/apache/spark/pull/39728#issuecomment-1404649043
> Could you reply on @attilapiros 's comment, @holdenk ?
Done :)
--
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 #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39728:
URL: https://github.com/apache/spark/pull/39728#issuecomment-1402912399
cc @mridulm , too
--
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] attilapiros commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "attilapiros (via GitHub)" <gi...@apache.org>.
attilapiros commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1087190921
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1109,6 +1109,24 @@ private[spark] object Utils extends Logging {
}
}
+ /**
+ * Normalize IPv6 IPs and no-op on all other hosts.
+ */
+ private[spark] def normalizeIpIfNeeded(host: String): String = {
+ // Is this a v6 address. We ask users to add [] around v6 addresses as strs but
+ // there not always there. If it's just 0-9 and : and [] we treat it as a v6 address.
+ // This means some invalid addresses are treated as v6 addresses, but since they are
+ // not valid hostnames it doesn't matter.
+ // See https://www.rfc-editor.org/rfc/rfc1123#page-13 for context around valid hostnames.
+ val addressRe = """^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""".r
Review Comment:
If the v6 address ends with 0's and it is shortened then the regexp will fail, isn't it?
```
scala> "[2:7::]".matches("""^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""")
res1: Boolean = false
```
Or the example from [this comment](https://www.reddit.com/r/networking/comments/49eap8/comment/d0r42ww/?utm_source=reddit&utm_medium=web2x&context=3):
```
scala> "[2600::]".matches("""^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""")
res2: Boolean = false
```
--
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] holdenk commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1086106561
##########
core/src/main/scala/org/apache/spark/rpc/RpcAddress.scala:
##########
@@ -23,30 +23,37 @@ import org.apache.spark.util.Utils
/**
* Address for an RPC environment, with hostname and port.
*/
-private[spark] case class RpcAddress(_host: String, port: Int) {
-
- lazy val host: String = Utils.addBracketsIfNeeded(_host)
+private[spark] case class RpcAddress(host: String, port: Int) {
def hostPort: String = host + ":" + port
/** Returns a string in the form of "spark://host:port". */
def toSparkURL: String = "spark://" + hostPort
override def toString: String = hostPort
+
+
Review Comment:
Sure :)
--
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] holdenk commented on pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #39728:
URL: https://github.com/apache/spark/pull/39728#issuecomment-1405342034
Thanks y'all :)
--
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] holdenk commented on a diff in pull request #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #39728:
URL: https://github.com/apache/spark/pull/39728#discussion_r1087508879
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1118,7 +1118,7 @@ private[spark] object Utils extends Logging {
// This means some invalid addresses are treated as v6 addresses, but since they are
// not valid hostnames it doesn't matter.
// See https://www.rfc-editor.org/rfc/rfc1123#page-13 for context around valid hostnames.
- val addressRe = """^\[{0,1}([0-9:]+?:[0-9]+)\]{0,1}$""".r
+ val addressRe = """^\[{0,1}([0-9:]+?:[0-9]*)\]{0,1}$""".r
Review Comment:
So that won't guarantee that there is at least one : in it, which we want since ":" is not a valid part of a hostname but is required for a v6 address.
--
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 #39728: [SPARK-42173][CORE] RpcAddress equality can fail
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39728:
URL: https://github.com/apache/spark/pull/39728#issuecomment-1405330138
Thank you, @holdenk , @mridulm , @attilapiros . Let me merge this to master/branch-3.4 to complete [SPARK-39457 Support IPv6-only environment in Apache Spark 3.4](https://issues.apache.org/jira/browse/SPARK-39457). I'll test more during the QA period.
--
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