You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "rednaxelafx (via GitHub)" <gi...@apache.org> on 2023/10/03 16:53:10 UTC
Re: [PR] [SPARK-45136][CONNECT] Enhance ClosureCleaner with Ammonite support [spark]
rednaxelafx commented on code in PR #42995:
URL: https://github.com/apache/spark/pull/42995#discussion_r1332183182
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -46,11 +42,24 @@ private[spark] object ClosureCleaner extends Logging {
null
} else {
val baos = new ByteArrayOutputStream(128)
- Utils.copyStream(resourceStream, baos, true)
+
+ SparkStreamUtils.copyStream(resourceStream, baos, closeStreams = true)
new ClassReader(new ByteArrayInputStream(baos.toByteArray))
}
}
+ private[util] def isAmmoniteCommandOrHelper(clazz: Class[_]): Boolean = clazz.getName.matches(
+ "^ammonite\\.\\$sess\\.cmd[0-9]*(\\$Helper\\$?)?")
+
+ private[util] def isDefinedInAmmonite(clazz: Class[_]): Boolean = clazz.getName.matches(
+ "^ammonite\\.\\$sess\\.cmd[0-9]*.*")
Review Comment:
Ditto
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -893,10 +1101,12 @@ private class InnerClosureFinder(output: Set[Class[_]]) extends ClassVisitor(ASM
op: Int, owner: String, name: String, desc: String, itf: Boolean): Unit = {
val argTypes = Type.getArgumentTypes(desc)
if (op == INVOKESPECIAL && name == "<init>" && argTypes.length > 0
- && argTypes(0).toString.startsWith("L") // is it an object?
- && argTypes(0).getInternalName == myName) {
- output += Utils.classForName(owner.replace('/', '.'),
- initialize = false, noSparkClassLoader = true)
+ && argTypes(0).toString.startsWith("L") // is it an object?
+ && argTypes(0).getInternalName == myName) {
Review Comment:
Is this format change necessary? I like the original better...
(If this is from scalafmt then I have no problem with it 🤷 )
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -46,11 +42,24 @@ private[spark] object ClosureCleaner extends Logging {
null
} else {
val baos = new ByteArrayOutputStream(128)
- Utils.copyStream(resourceStream, baos, true)
+
+ SparkStreamUtils.copyStream(resourceStream, baos, closeStreams = true)
new ClassReader(new ByteArrayInputStream(baos.toByteArray))
}
}
+ private[util] def isAmmoniteCommandOrHelper(clazz: Class[_]): Boolean = clazz.getName.matches(
+ "^ammonite\\.\\$sess\\.cmd[0-9]*(\\$Helper\\$?)?")
Review Comment:
Can we use Scala's triple quote string literal here to avoid the need for the escaping?
i.e.
```scala
"""^ammonite\.\$sess\.cmd[0-9]*(\$Helper\$?)?"""
```
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -893,10 +1101,12 @@ private class InnerClosureFinder(output: Set[Class[_]]) extends ClassVisitor(ASM
op: Int, owner: String, name: String, desc: String, itf: Boolean): Unit = {
val argTypes = Type.getArgumentTypes(desc)
if (op == INVOKESPECIAL && name == "<init>" && argTypes.length > 0
- && argTypes(0).toString.startsWith("L") // is it an object?
- && argTypes(0).getInternalName == myName) {
- output += Utils.classForName(owner.replace('/', '.'),
- initialize = false, noSparkClassLoader = true)
+ && argTypes(0).toString.startsWith("L") // is it an object?
+ && argTypes(0).getInternalName == myName) {
Review Comment:
Is this format change necessary? I like the original better...
(If this is from scalafmt then I have no problem with it 🤷 )
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -678,18 +851,45 @@ private[spark] object IndylambdaScalaClosures extends Logging {
// Depth-first search for inner closures and track the fields that were accessed in them.
// Start from the lambda body's implementation method, follow method invocations
val visited = Set.empty[MethodIdentifier[_]]
- val stack = Stack[MethodIdentifier[_]](implMethodId)
+ val queue = Queue[MethodIdentifier[_]](implMethodId)
Review Comment:
Is this intended? What's the rationale for changing from stack => queue? Is breadth-first search preferred here?
--
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