You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2023/04/25 11:44:27 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
LuciferYang opened a new pull request, #40945:
URL: https://github.com/apache/spark/pull/40945
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
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] sunchao commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1192565640
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
+1 on removing the reflection
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191838807
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -26,6 +26,7 @@ import java.util.{Date, Locale}
import scala.collection.JavaConverters._
import scala.collection.mutable
import scala.collection.mutable.HashMap
+import scala.language.existentials
Review Comment:
for fix compile error of
```
[error] /core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:575:36: inferred existential type org.apache.hadoop.fs.FSDataOutputStreamBuilder[S,B]( forSome { type S <: org.apache.hadoop.fs.FSDataOutputStream; type B <: org.apache.hadoop.fs.FSDataOutputStreamBuilder[S,B] }), which cannot be expressed by wildcards, should be enabled
[error] by making the implicit value scala.language.existentials visible.
[error] ----
[error] This can be achieved by adding the import clause 'import scala.language.existentials'
[error] or by setting the compiler option -language:existentials.
[error] See the Scaladoc for value scala.language.existentials for a discussion
[error] why the feature should be explicitly enabled.
[error] val builder = fs.createFile(qualifiedPath)
[error] ^
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 19 s, completed 2023-5-12 10:05:06
```
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1176397320
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
I originally intended to fix it as follows:
```scala
fs.createFile(qualifiedPath) match {
case hb: HdfsDataOutputStreamBuilder => hb.replicate().build()
case _ => fs.create(path)
}
```
But I'm not sure if there are other types of `FSDataOutputStreamBuilder` that support the `replicate` method. Do you have any better suggestions? @pan3793 @sunchao
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191863241
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
The intent here is not to invoke any random `replicate` method which might be sitting inside the `FSDataOutputStreamBuilder` - but to do so specifically for hdfs `HdfsDataOutputStreamBuilder`/subclasses.
Unfortunately, this is not yet resolved (see HDFS-14038).
See [this discussion](https://github.com/apache/spark/pull/22881#discussion_r229102457) for more.
As part of this fix, if there are other implementations which get broken due to this change, we should address them appropriately on a case by case basis or get hdfs team to fix their interfaces : not keep a broken contract.
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191804004
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
QQ: If this is specific to `HdfsDataOutputStreamBuilder` and we are expecting to invoke `replicate` on that class/sub-classes, why not tighten the code to do it specifically for that class (and its subclasses) ?
With hadoop 2.x, I can understand the generic replicate method invocation - but we have the opportunity to be more nuanced now.
I am fine with move that snippet to a different PR though.
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40945:
URL: https://github.com/apache/spark/pull/40945#issuecomment-1546308609
Thank you 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] LuciferYang commented on pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40945:
URL: https://github.com/apache/spark/pull/40945#issuecomment-1546300263
Thanks @sunchao @mridulm @pan3793
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1190187898
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
friendly ping @sunchao
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191804004
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
QQ: If this is specific to `HdfsDataOutputStreamBuilder` and we are expecting to invoke `replicate` on that class/sub-classes, why not tighten the code to do it specifically for that class (and its subclasses) ?
With hadoop 2.x, I can understand the generic replicate method invocation - but we have the opportunity to be more specific now.
I am fine with move that snippet to a different PR though.
--
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] pan3793 commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191868486
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
that makes sense, thanks @mridulm for detailed explanation
--
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] sunchao closed pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao closed pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
URL: https://github.com/apache/spark/pull/40945
--
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] sunchao commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191750664
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
I think this method is very private and only available for `HdfsDataOutputStreamBuilder`. I'm OK if it stay as it is.
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -26,6 +26,7 @@ import java.util.{Date, Locale}
import scala.collection.JavaConverters._
import scala.collection.mutable
import scala.collection.mutable.HashMap
+import scala.language.existentials
Review Comment:
hmm what is this for?
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1192789111
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
hmm... I still want to put it in an independent pr. WDYT @pan3793 @mridulm
--
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] pan3793 commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1176406342
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
I'm +1 on the existing reflection approach since it can support any implementation which supports the `replicate` method.
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191865409
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
> As I mentioned before, we can move that to a separate PR though - to derisk this specific PR (in case we do find credible reasons to revert/fix the change)
Agreed
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1192789111
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
hmm... I still want to put it in an independent 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] LuciferYang commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191844146
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
> I think this method is very private and only available for `HdfsDataOutputStreamBuilder`. I'm OK if it stay as it is.
Can we confirm this?
My concern is if there is a `XDataOutputStreamBuilder`(Maybe user specific implementation), if `XDataOutputStreamBuilder` is subclass of `FSDataOutputStreamBuilder ` instead of `HdfsDataOutputStreamBuilder`, but `XDataOutputStreamBuilder` also happens to have `replicate ` method.
In this scenario, the current way can be executed, but changing to similar to
```
fs.createFile(qualifiedPath) match {
case hb: HdfsDataOutputStreamBuilder => hb.replicate().build()
case _ => fs.create(path)
}
```
will inadvertently disrupt this behavior, so I let it stay as it is 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] LuciferYang commented on a diff in pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1176397320
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
I originally intended to modify it as follows:
```scala
fs.createFile(qualifiedPath) match {
case hb: HdfsDataOutputStreamBuilder => hb.replicate().build()
case _ => fs.create(path)
}
```
But I'm not sure if there are other types of `FSDataOutputStreamBuilder` that support the `replicate` method. Do you have any better suggestions? @pan3793 @sunchao
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191863895
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
In other words, I would argue to make the change proposed and remove the reflection:
```
fs.createFile(qualifiedPath) match {
case hb: HdfsDataOutputStreamBuilder => hb.replicate().build()
case _ => fs.create(path)
}
```
As I mentioned before, we can move that to a separate PR though - to derisk this specific PR (in case we do find credible reasons to revert/fix the change)
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1192801468
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
> +1 on removing the reflection
testing, will give a new one later
--
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 #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #40945:
URL: https://github.com/apache/spark/pull/40945#discussion_r1191804004
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -623,15 +623,13 @@ private[spark] object SparkHadoopUtil extends Logging {
fs.create(path)
} else {
try {
- // Use reflection as this uses APIs only available in Hadoop 3
- val builderMethod = fs.getClass().getMethod("createFile", classOf[Path])
// the builder api does not resolve relative paths, nor does it create parent dirs, while
// the old api does.
if (!fs.mkdirs(path.getParent())) {
throw new IOException(s"Failed to create parents of $path")
}
val qualifiedPath = fs.makeQualified(path)
- val builder = builderMethod.invoke(fs, qualifiedPath)
+ val builder = fs.createFile(qualifiedPath)
val builderCls = builder.getClass()
// this may throw a NoSuchMethodException if the path is not on hdfs
val replicateMethod = builderCls.getMethod("replicate")
Review Comment:
QQ: If this is specific to `HdfsDataOutputStreamBuilder` and we are expecting to invoke `replicate` on that specific class, why not tighten the code to do it specifically for that class (and its subclasses) ?
With hadoop 2.x, I can understand the generic replicate method invocation - but we have the opportunity to be more specific now.
I am fine with move that snippet to a different PR though.
--
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] sunchao commented on pull request #40945: [SPARK-43272][CORE] Directly call `createFile` instead of reflection
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #40945:
URL: https://github.com/apache/spark/pull/40945#issuecomment-1546299221
Thanks, merged to master. Test failure unrelated.
--
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