-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add fix for metricsProperties when HasPrometheusConfigFile is true. #847
add fix for metricsProperties when HasPrometheusConfigFile is true. #847
Conversation
d143557
to
d1ddea8
Compare
54a4d33
to
e6345c5
Compare
e6345c5
to
2461216
Compare
@liyinan926 Please review. There are auto-generate code for previous PRs such as lifeCycle, TerminationGracePeriod |
The authors of the previous PRs must have forgotten to run these. Please include them in this PR. |
823fe63
to
4112c80
Compare
@@ -557,6 +557,10 @@ type MonitoringSpec struct { | |||
// +optional | |||
// If not specified, the content in spark-docker/conf/metrics.properties will be used. | |||
MetricsProperties *string `json:"metricsProperties,omitempty"` | |||
// MetricsPropertiesFile is the address of file metrics.properties for configuring the Spark metric system. |
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.
Suggest change address
to the container local path
.
port, configFile) | ||
} else { | ||
glog.V(2).Infof("Using the default Prometheus configuration.") | ||
if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() { |
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.
Can we refactor the if-else block like the following to make it easier to read and understand?
var javaOption string
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
glog.V(2).Infof("Loading local metrics and Prometheus configuration files")
...
javaOptions = ...
} else {
glog.V(2).Infof("Creating a ConfigMap for metrics and/or Prometheus configurations")
...
javaOptions = ...
}
if configMap.Data[metricsPropertiesKey] != test.metricsProperties { | ||
if test.app.Spec.Monitoring.Prometheus.ConfigFile != nil && | ||
test.app.Spec.Monitoring.MetricsPropertiesFile == nil && | ||
len(configMap.Data) > 1 { |
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.
len(configMap.Data) != 1
|
||
if test.app.Spec.Monitoring.Prometheus.ConfigFile == nil && | ||
test.app.Spec.Monitoring.MetricsPropertiesFile != nil && | ||
len(configMap.Data) > 1 { |
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.
len(configMap.Data) != 1
…c in monitoring_config.go.
@liyinan926 Please review again, thanks |
if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() { | ||
glog.V(2).Infof("Loading Prometheus configuration.") | ||
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() { | ||
glog.V(2).Infof("Loading local Spark metrics and Prometheus configuration files, not creating configMap with default properties.") |
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.
The code blocks below should be moved into this if block:
if app.HasPrometheusConfigFile() {
configFile := *app.Spec.Monitoring.Prometheus.ConfigFile
glog.V(2).Infof("Overriding the default Prometheus configuration with config file %s in the Spark image.", configFile)
javaOption = fmt.Sprintf("-javaagent:%s=%d:%s", app.Spec.Monitoring.Prometheus.JmxExporterJar,
port, configFile)
}
if app.HasMetricsPropertiesFile() {
app.Spec.SparkConf["spark.metrics.conf"] = *app.Spec.Monitoring.MetricsPropertiesFile
}
And in the else block, generate javaOptions
the other way.
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.
Still WIP.
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 think I will set the logic back to the original. with clarify comments.
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
} else {
}
not really cover all the cases. to cover all the case:
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
Overwtie defaul spark metrics file & defaul prometheus config file
} else if app.HasMetricsPropertiesFile() {
Overwtie defaul spark metrics file & create CM with default prometheus config file
} else if app.HasPrometheusConfigFile(){
Overwtie defaul prometheus config file & create CM with default spark metrics file
} else {
create CM with default spark metrics file & defaul prometheus config file
}
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 think you can combine the three else
cases into one, as you already handle the metrics config file and Prometheus config file individually in buildPrometheusConfigMap
. So basically there are cases: 1) no metrics config file nor Prometheus config file are specified, and 2) at least one of the two config files is not specified, and the ConfigMap needs to be created.
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.
If I do that. there will be places for duplicate code blocks. As a example here:
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
// using PrometheusConfigFile
javaOption = ...
// using MetricsPropertiesFile
app.Spec.SparkConf["spark.metrics.conf"] = ...
} else {
// Create configMap
configMap := buildPrometheusConfigMap(app, configMapName)
if app.HasPrometheusConfigFile() {
// using PrometheusConfigFile
javaOption = ...
} else {
// using default PrometheusConfigFile
javaOption = ...
}
// using MetricsPropertiesFile
if app.HasMetricsPropertiesFile() {
app.Spec.SparkConf["spark.metrics.conf"] = ...
}
}
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.
OK, let's just keep the current structure in the PR.
4c2c23f
to
8fdcbf2
Compare
8fdcbf2
to
3ca28f2
Compare
// If one of the metricsPropertiesFile & prometheusConfigFile is not set | ||
// mounting configMap to using default configuration | ||
if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() { | ||
glog.V(2).Infof("Loading Prometheus configuration.") |
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.
Can we change this to Creating a ConfigMap for metrics and Prometheus configurations
?
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 meant the log message, not the comment :).
// If one of the metricsPropertiesFile & prometheusConfigFile is not set | ||
// mounting configMap to using default configuration | ||
if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() { | ||
glog.V(2).Infof("Loading Prometheus configuration.") |
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 meant the log message, not the comment :).
port, | ||
config.PrometheusConfigMapMountPath, | ||
prometheusConfigKey) | ||
glog.V(2).Infof("Setting the default Prometheus configuration.") |
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.
Please also remove this log line that is pretty useless.
94439fb
to
993d011
Compare
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.
LGTM. Thanks for contributing!
Fix the bug where Monitor.Prometheus.ConfigFile is set. the configMap which contains spark metrics.properties & prometheus config file will not be created & mounted, so the metricsProperties will be useless.
Add a new property metricsPropertiesFile, this will overwrite the value
spark.metrics.conf
in spark.properties. give a better way to specify spark metrics configuration when it is large.