[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792679337



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -595,7 +593,7 @@ private[spark] object Utils extends Logging {
 if (lowerSrc.endsWith(".jar")) {
   RunJar.unJar(source, dest, RunJar.MATCH_ANY)
 } else if (lowerSrc.endsWith(".zip")) {
-  FileUtil.unZip(source, dest)
+  unZip(source, dest)

Review comment:
   Is it possible to chmod again after unzip? I'm not 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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792673852



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -605,6 +603,22 @@ private[spark] object Utils extends Logging {
 }
   }
 
+  def unZip(inFile: File, untarDir: File): Unit = {
+val untarCommand = new StringBuffer

Review comment:
   There will be no concurrency issue here
   
   

##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -605,6 +603,22 @@ private[spark] object Utils extends Logging {
 }
   }
 
+  def unZip(inFile: File, untarDir: File): Unit = {
+val untarCommand = new StringBuffer

Review comment:
   There will be no concurrency issue
   
   




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792670450



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -605,6 +603,29 @@ private[spark] object Utils extends Logging {
 }
   }
 
+  def unZip(inFile: File, unzipDir: File): Unit = {
+if (!unzipDir.mkdirs && !unzipDir.isDirectory) {
+  throw new IOException("Mkdirs failed to create " + unzipDir)
+} else {
+  if (Shell.WINDOWS) {
+FileUtil.unZip(inFile, unzipDir)

Review comment:
   +1 agree with @HyukjinKwon 
   
   




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792668929



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -3196,8 +3217,8 @@ private[spark] object Utils extends Logging {
 entry = in.getNextEntry()
   }
   in.close() // so that any error in closing does not get ignored
-  logInfo(s"Unzipped from $dfsZipFile\n\t${files.mkString("\n\t")}")
 } finally {
+  logInfo(s"Unzipped from $dfsZipFile\n\t${files.mkString("\n\t")}")

Review comment:
   This log should be printed after successful decompression. It doesn't 
seem appropriate to move it in finally
   
   




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792668929



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -3196,8 +3217,8 @@ private[spark] object Utils extends Logging {
 entry = in.getNextEntry()
   }
   in.close() // so that any error in closing does not get ignored
-  logInfo(s"Unzipped from $dfsZipFile\n\t${files.mkString("\n\t")}")
 } finally {
+  logInfo(s"Unzipped from $dfsZipFile\n\t${files.mkString("\n\t")}")

Review comment:
   This log should be printed after successful decompression. It doesn't 
seem appropriate to put it in finally
   
   




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792667649



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -55,14 +53,14 @@ import org.apache.hadoop.conf.Configuration
 import org.apache.hadoop.fs.{FileSystem, FileUtil, Path}
 import org.apache.hadoop.io.compress.{CompressionCodecFactory, 
SplittableCompressionCodec}
 import org.apache.hadoop.security.UserGroupInformation
-import org.apache.hadoop.util.{RunJar, StringUtils}
+import org.apache.hadoop.util.Shell.ShellCommandExecutor
+import org.apache.hadoop.util.{RunJar, Shell, StringUtils}
 import org.apache.hadoop.yarn.conf.YarnConfiguration
 import org.eclipse.jetty.util.MultiException
 import org.slf4j.Logger
-
 import org.apache.spark._
 import org.apache.spark.deploy.SparkHadoopUtil
-import org.apache.spark.internal.{config, Logging}

Review comment:
   Please restore to `import org.apache.spark.internal.{config, Logging}`
   
   

##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -55,14 +53,14 @@ import org.apache.hadoop.conf.Configuration
 import org.apache.hadoop.fs.{FileSystem, FileUtil, Path}
 import org.apache.hadoop.io.compress.{CompressionCodecFactory, 
SplittableCompressionCodec}
 import org.apache.hadoop.security.UserGroupInformation
-import org.apache.hadoop.util.{RunJar, StringUtils}
+import org.apache.hadoop.util.Shell.ShellCommandExecutor
+import org.apache.hadoop.util.{RunJar, Shell, StringUtils}
 import org.apache.hadoop.yarn.conf.YarnConfiguration
 import org.eclipse.jetty.util.MultiException
 import org.slf4j.Logger
-

Review comment:
   ditto




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] LuciferYang commented on a change in pull request #35278: [SPARK-37677][CORE] Use the shell command to decompress the ZIP file

2022-01-26 Thread GitBox 


LuciferYang commented on a change in pull request #35278:
URL: https://github.com/apache/spark/pull/35278#discussion_r792666914



##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -32,7 +32,6 @@ import java.util.{Locale, Properties, Random, UUID}
 import java.util.concurrent._
 import java.util.concurrent.TimeUnit.NANOSECONDS
 import java.util.zip.{GZIPInputStream, ZipInputStream}
-

Review comment:
   This blank line should not be deleted

##
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##
@@ -42,7 +41,6 @@ import scala.reflect.ClassTag
 import scala.util.{Failure, Success, Try}
 import scala.util.control.{ControlThrowable, NonFatal}
 import scala.util.matching.Regex
-

Review comment:
   ditto




-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org