Skip to content
This repository has been archived by the owner on Jan 9, 2020. It is now read-only.

Docker logging handler #576

Open
wants to merge 13 commits into
base: branch-2.2-kubernetes
Choose a base branch
from
Open
11 changes: 11 additions & 0 deletions resource-managers/kubernetes/integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

<artifactId>spark-kubernetes-integration-tests_2.11</artifactId>
<properties>
<slf4j-log4j12.version>1.7.24</slf4j-log4j12.version>
<sbt.project.name>kubernetes-integration-tests</sbt.project.name>
</properties>
<packaging>jar</packaging>
Expand All @@ -42,6 +43,16 @@
<artifactId>spark-core_${scala.binary.version}</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
<version>${slf4j-log4j12.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
</dependency>
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-kubernetes_${scala.binary.version}</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,14 @@
# limitations under the License.
#

# Set everything to be logged to the file target/integration-tests.log
log4j.rootCategory=INFO, file
# Set everything to be logged to the file target/integration-tests.log and the console
log4j.rootCategory=INFO, file, stdout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want console here. On the upside, I understand most people actually do not know they can check target/integration-tests.log. On the downside, too many log messages could get people lost.

Anyone has a strong opinion on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping! @echarles or anyone else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I am good with this PR pending this question being resolved. Maybe we just want to remove console output part and move on so the PR can be merged?


log4j.appender.stdout=org.apache.log4j.ConsoleAppender
log4j.appender.stdout.Target=System.out
log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
log4j.appender.stdout.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n

log4j.appender.file=org.apache.log4j.FileAppender
log4j.appender.file.append=true
log4j.appender.file.file=target/integration-tests.log
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.spark.deploy.k8s.integrationtest

import org.apache.log4j.{Logger, LogManager, Priority}

trait Logging {

private val log: Logger = LogManager.getLogger(this.getClass)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Spark already has something similiar in core?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I have seen that also. Does it make sense to follow up with this PR and the apache repo?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible let's reuse what Spark has, the same question will arise when we try to merge back to Spark as well.


protected def logDebug(msg: => String) = if (log.isDebugEnabled) log.debug(msg)

protected def logInfo(msg: => String) = if (log.isInfoEnabled) log.info(msg)

protected def logWarning(msg: => String) = if (log.isEnabledFor(Priority.WARN)) log.warn(msg)

protected def logWarning(msg: => String, throwable: Throwable) =
if (log.isEnabledFor(Priority.WARN)) log.warn(msg, throwable)

protected def logError(msg: => String) = if (log.isEnabledFor(Priority.ERROR)) log.error(msg)
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,13 @@ import java.net.URI
import java.nio.file.Paths

import scala.collection.JavaConverters._

import com.spotify.docker.client.{DefaultDockerClient, DockerCertificates, LoggingBuildHandler}
import org.apache.http.client.utils.URIBuilder
import org.apache.spark.deploy.k8s.integrationtest.Logging
import org.scalatest.concurrent.{Eventually, PatienceConfiguration}
import org.scalatest.time.{Minutes, Seconds, Span}

import org.apache.spark.internal.Logging
import org.apache.spark.util.RedirectThread



private[spark] class SparkDockerImageBuilder
(private val dockerEnv: Map[String, String]) extends Logging{

Expand Down Expand Up @@ -113,10 +109,13 @@ private[spark] class SparkDockerImageBuilder
}

private def buildImage(name: String, dockerFile: String): Unit = {
logInfo(s"Start building docker image $name from Dockerfile $dockerFile")
dockerClient.build(
DOCKER_BUILD_PATH,
name,
dockerFile,
new LoggingBuildHandler())
logInfo(s"Built $name docker image")
}

}