-
Notifications
You must be signed in to change notification settings - Fork 117
Docker logging handler #576
base: branch-2.2-kubernetes
Are you sure you want to change the base?
Changes from 7 commits
118f587
fdd9b5c
6c8851c
21f5223
9ab9b02
fec57fc
a55d8b2
25cd876
742520a
ea3d942
7451bf6
5ee6eaa
3307860
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Spark already has something similiar in core? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* 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.docker | ||
|
||
import com.spotify.docker.client.ProgressHandler | ||
import com.spotify.docker.client.exceptions.DockerException | ||
import com.spotify.docker.client.messages.ProgressMessage | ||
|
||
class LoggingBuildHandler() extends ProgressHandler { | ||
@throws[DockerException] | ||
def progress(message: ProgressMessage) { | ||
if (message.error != null) throw new DockerException(message.toString) | ||
else if (message.status == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the null status mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is not much in the spotify source. It just tells
I came to this trying to find a correct level of output. |
||
println("build: {}", message) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid using println and use logging API instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just pushed a new commit using |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?