Restructure the test backends #248

foxish · 2017-04-27T18:33:25Z

This merges into #243

foxish · 2017-04-28T19:17:51Z

@mccheah Could you please review this PR and #243 which this merges into? They're fairly small and should unblock some e2es tests I want to setup against GCE.

mccheah

Mostly minor changes. The overall approach is good.

mccheah · 2017-04-28T20:25:36Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

+import org.apache.spark.deploy.kubernetes.integrationtest.constants.GCE_TEST_BACKEND
+
+class GCETestBackend extends IntegrationTestBackend {
+  val master = System.getProperty("spark.kubernetes.test.master")


nit: Mark these as private

Pass the system property through the constructor.

mccheah · 2017-04-28T20:25:50Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

+  val defaultClient = new DefaultKubernetesClient(k8ClientConfig)
+
+  override def getKubernetesClient: DefaultKubernetesClient = {
+    return defaultClient


No need for return keyword here. The return value could be on the same line as the function signature, without braces.

mccheah · 2017-04-28T20:27:50Z

...cala/org/apache/spark/deploy/kubernetes/integrationtest/backend/IntegrationTestBackend.scala

+
+object IntegrationTestBackendFactory {
+  def getTestBackend: IntegrationTestBackend = {
+    Option(System.getProperty("spark.kubernetes.test.master")).map {


Pass the system property's value into the constructor of GCEBackend, and GCEBackend shouldn't look up the system property itself.

mccheah · 2017-04-28T20:34:05Z

...ests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesSuite.scala


  override def afterAll(): Unit = {
-    kubernetesTestClient.cleanUp()
+    testBackend.cleanUp


Nit: Use empty-paren for non-getter methods.

mccheah · 2017-04-28T20:35:13Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

+    return defaultClient
+  }
+
+  override def cleanUp: Unit = {}


Nit: Provide empty-paren for methods that aren't getters.

cleanup can have a default implementation in the trait that is empty, and then it only has to be overridden in MinikubeTestBackend.

mccheah · 2017-04-28T20:35:32Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

+  }
+
+  override def cleanUp: Unit = {}
+  override def name: String = GCE_TEST_BACKEND


Nit: Empty-paren.

mccheah · 2017-04-28T20:36:47Z

...rg/apache/spark/deploy/kubernetes/integrationtest/backend/minikube/MinikubeTestBackend.scala

+import org.apache.spark.deploy.kubernetes.integrationtest.docker.SparkDockerImageBuilder
+
+class MinikubeTestBackend extends IntegrationTestBackend {
+  Minikube.startMinikube()


Prefer if these were not set up in the constructor. Instead, IntegrationTestBackend should have an initialize method that the Minikube variant implements. Then, call initialize in beforeAll().

mccheah · 2017-04-28T20:37:07Z

...cala/org/apache/spark/deploy/kubernetes/integrationtest/backend/IntegrationTestBackend.scala

+import org.apache.spark.deploy.kubernetes.integrationtest.backend.minikube.{Minikube, MinikubeTestBackend}
+import org.apache.spark.deploy.kubernetes.integrationtest.docker.SparkDockerImageBuilder
+
+abstract class IntegrationTestBackend {


trait preferred over abstract class.

mccheah · 2017-04-28T20:39:01Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

+  var k8ConfBuilder = new ConfigBuilder()
+    .withApiVersion("v1")
+    .withMasterUrl(resolveK8sMaster(master))
+  val k8ClientConfig = k8ConfBuilder.build


No need to have a separate var for the builder - simply set the config val inline:

mccheah · 2017-04-28T20:40:57Z

...ests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesSuite.scala


 private[spark] class KubernetesSuite extends SparkFunSuite {
-  private var kubernetesTestClient: KubernetesTestClient = _
+  private var testBackend: IntegrationTestBackend = _


If initialization is done in a separate method and not the constructor, then this can become a val and set inline here.

Maybe I'm misunderstanding this. I still need to keep a reference to it to later cleanUp after the tests correct?

Right but this can be a val instead of a var that's set in beforeAll():

private val testBackend = IntegrationTestBackendFactory...

Ah! Got it. Thanks. Fixed.

mccheah · 2017-04-29T00:27:07Z

...st/scala/org/apache/spark/deploy/kubernetes/integrationtest/backend/GCE/GCETestBackend.scala

-  val k8ClientConfig = k8ConfBuilder.build
-  val defaultClient = new DefaultKubernetesClient(k8ClientConfig)
+private[spark] class GCETestBackend(val master: String) extends IntegrationTestBackend {
+  private var defaultClient: DefaultKubernetesClient = _


In the GCE case it's probably fine to initialize in the constructor - create a KubernetesClient should be fairly lightweight. What we wanted to avoid in the Minikube case was to have side effects in the constructor.

mccheah

Looks ok to merge into the dependent PR. We can continue reviewing on the downstream.

* Part 1: making test code cluster-agnostic * Final checked * Move all test code into KubernetesTestComponents * Addressed comments * Fixed doc * Restructure the test backends (#248) * Restructured the test backends * Address comments * var -> val * Comments * removed deadcode

* Part 1: making test code cluster-agnostic * Final checked * Move all test code into KubernetesTestComponents * Addressed comments * Fixed doc * Restructure the test backends (apache-spark-on-k8s#248) * Restructured the test backends * Address comments * var -> val * Comments * removed deadcode (cherry picked from commit 6b489c2)

Resync with apache-spark-on-k8s upstream

* Part 1: making test code cluster-agnostic * Final checked * Move all test code into KubernetesTestComponents * Addressed comments * Fixed doc * Restructure the test backends (apache-spark-on-k8s#248) * Restructured the test backends * Address comments * var -> val * Comments * removed deadcode

Restructured the test backends

6dd476d

foxish mentioned this pull request Apr 27, 2017

Enable testing against GCE clusters #243

Merged

mccheah suggested changes Apr 28, 2017

View reviewed changes

Address comments

81fdff2

mccheah reviewed Apr 29, 2017

View reviewed changes

var -> val

8be3a1f

mccheah approved these changes Apr 29, 2017

View reviewed changes

mccheah merged commit d4d2e9c into multi-k8s-tests Apr 29, 2017

mccheah deleted the restructure-integration-test branch April 29, 2017 02:33

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019

Merge pull request apache-spark-on-k8s#248 from palantir/resync-kube

65f1048

Resync with apache-spark-on-k8s upstream

Restructure the test backends #248

Restructure the test backends #248

Uh oh!

Conversation

foxish commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish commented Apr 28, 2017

Uh oh!

mccheah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

foxish commented Apr 27, 2017 •

edited

Loading

mccheah Apr 29, 2017 •

edited

Loading