roczei commented on code in PR #533:
URL: https://github.com/apache/livy/pull/533#discussion_r3656956831
##########
server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala:
##########
@@ -68,6 +71,15 @@ object SparkYarnApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
+ /**
+ * Build a GetApplicationsRequest filtered by Spark application type and
tags.
+ */
+ private def createGetApplicationsRequest(appTags: util.Set[String]):
GetApplicationsRequest = {
Review Comment:
Pulling createGetApplicationsRequest() into a private helper is a nice idea
to avoid duplication.
##########
server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala:
##########
@@ -82,7 +94,13 @@ object SparkYarnApp extends Logging {
// kill the app if found it and remove it if exceeding a threshold
val iter = leakedAppTags.entrySet().iterator()
val now = System.currentTimeMillis()
- val apps = client.getApplications(appType).asScala
+ val tagSet = new util.HashSet[String]()
Review Comment:
The second iterator (entries) only exists to copy keys into a HashSet. This
can be simplified to:
```suggestion
val tagSet = new util.HashSet[String]()
val tagSet = new util.HashSet[String](leakedAppTags.keySet())
```
This is one line instead of four does the same thing and eliminates the
redundant iterator.
##########
server/src/test/scala/org/apache/livy/utils/SparkYarnAppSpec.scala:
##########
@@ -49,6 +50,13 @@ class SparkYarnAppSpec extends FunSpec with
LivyBaseUnitTestSuite {
Thread.`yield`()
}
+ private def mockGetApplicationsByTags(
+ client: YarnClient,
+ reports: List[ApplicationReport]): Unit = {
+ when(client.getApplications(any(classOf[GetApplicationsRequest])))
Review Comment:
Using `any(classOf[GetApplicationsRequest])` is a bit broad, it won't catch
bugs if wrong tags or app types are passed later. How about using an
ArgumentCaptor here to verify the exact fields? For example:
```
diff --git
a/server/src/test/scala/org/apache/livy/utils/SparkYarnAppSpec.scala
b/server/src/test/scala/org/apache/livy/utils/SparkYarnAppSpec.scala
index cfaca49..55d69c4 100644
--- a/server/src/test/scala/org/apache/livy/utils/SparkYarnAppSpec.scala
+++ b/server/src/test/scala/org/apache/livy/utils/SparkYarnAppSpec.scala
@@ -30,6 +30,7 @@ import
org.apache.hadoop.yarn.api.records.YarnApplicationState._
import org.apache.hadoop.yarn.client.api.YarnClient
import org.apache.hadoop.yarn.exceptions.ApplicationAttemptNotFoundException
import org.apache.hadoop.yarn.util.ConverterUtils
+import org.mockito.ArgumentCaptor
import org.mockito.Matchers.any
import org.mockito.Mockito._
import org.mockito.invocation.InvocationOnMock
@@ -411,6 +412,14 @@ class SparkYarnAppSpec extends FunSpec with
LivyBaseUnitTestSuite {
verify(mockYarnClient, atLeast(1)).getApplicationReport(appId)
verify(mockListener).appIdKnown(appId.toString)
+
+ val requestCaptor =
ArgumentCaptor.forClass(classOf[GetApplicationsRequest])
+ verify(mockYarnClient,
atLeast(1)).getApplications(requestCaptor.capture())
+ val capturedRequest = requestCaptor.getValue
+
assert(capturedRequest.getApplicationTags.contains(appTag.toLowerCase),
+ s"Request must contain the lowercase appTag
'${appTag.toLowerCase}'")
+ assert(capturedRequest.getApplicationTypes.contains("SPARK"),
+ "Request must filter by application type 'SPARK'")
}
}
}
@@ -685,11 +694,20 @@ class SparkYarnAppSpec extends FunSpec with
LivyBaseUnitTestSuite {
SparkYarnApp.init(livyConf, Some(client))
SparkYarnApp.leakedAppTags.clear()
- SparkYarnApp.leakedAppTags.put("leakApp",
System.currentTimeMillis())
+ val leakAppTag = "leakApp"
+ SparkYarnApp.leakedAppTags.put(leakAppTag,
System.currentTimeMillis())
Eventually.eventually(Eventually.timeout(TEST_TIMEOUT),
Eventually.interval(100 millis)) {
assert(SparkYarnApp.leakedAppTags.size() == 0)
}
+
+ val requestCaptor =
ArgumentCaptor.forClass(classOf[GetApplicationsRequest])
+ verify(client, atLeast(1)).getApplications(requestCaptor.capture())
+ val capturedRequest = requestCaptor.getValue
+
assert(capturedRequest.getApplicationTags.contains(leakAppTag.toLowerCase),
+ "Leaked app GC request must include the leaked app tag")
+ assert(capturedRequest.getApplicationTypes.contains("SPARK"),
+ "Leaked app GC request must filter by application type 'SPARK'")
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
