[SPARK-48984]Add Controller Metrics System and Utils #23

jiangzho · 2024-07-24T00:22:25Z

What changes were proposed in this pull request?

This is a breakdown PR of #12 - defines metrics system and utils classes to be used by the reconcilers.

It also refactors a few methods in previous utils class org.apache.spark.k8s.operator.reconciler.SparkReconcilerUtils into common utils package.

Why are the changes needed?

Breakdown PRs help us to move with more flexibility.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CIs

Was this patch authored or co-authored using generative AI tooling?

No

dongjoon-hyun

Thank you for making a spin-off.

BTW, please write the PR description as complete as a standalone PR next time, @jiangzho .

dongjoon-hyun · 2024-07-24T01:56:01Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+          .enableDynamicOverride(true)
+          .description(
+              "Comma-separated list of namespaces that the operator would be watching for "
+                  + "Spark resources. If set to '*' or unset, operator would watch all namespaces.")


Thank you for supporting this syntax.

I saw the implementation.

if ("*".equals(str)) { return Collections.emptySet(); }

Although I understand that you meant the following, the description about unset could be a little misleading because this configuration has a default value, default.

spark.kubernetes.operator.watchedNamespaces=* spark.kubernetes.operator.watchedNamespaces= # blank

Yep! I was hesitating about the 'unset' (which is under the hood can be considered as not setting namespaces), but indeed it can be quite confusing at properties level. I'd update this to advice user use * only instead of unset.

dongjoon-hyun · 2024-07-24T01:57:58Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+          .key("spark.kubernetes.operator.reconciler.foregroundRequestTimeoutSeconds")
+          .enableDynamicOverride(true)
+          .description(
+              "Timeout (in seconds) to for requests made to API server. this "


this -> This

dongjoon-hyun · 2024-07-24T02:00:16Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+          .defaultValue(30L)
+          .build();
+
+  public static final ConfigOption<Long> SPARK_APP_RECONCILE_INTERVAL_SECONDS =


Can we keep the pattern consistently? For example, since the name is spark.kubernetes.operator.reconciler,
SPARK_APP_RECONCILE_INTERVAL_SECONDS -> RECONCILER_SPARK_APP_INTERVAL_SECONDS?

dongjoon-hyun · 2024-07-24T02:01:15Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+                  + "updated. This interval controls the reconcile behavior of operator "
+                  + "reconciliation even when there's no update on SparkApplication, e.g. to "
+                  + "determine whether a hanging app needs to be proactively terminated. Thus "
+                  + "this is recommended to set to above 2 min to avoid unnecessary no-op "


min -> minutes

dongjoon-hyun · 2024-07-24T02:02:43Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+
+  public static final ConfigOption<Boolean> TRIM_ATTEMPT_STATE_TRANSITION_HISTORY =
+      ConfigOption.<Boolean>builder()
+          .key("spark.kubernetes.operator.reconciler.trimStateTransitionHistoryEnabled")


To use Enabled here and spark.kubernetes.operator.metrics.clientMetricsEnabled, we need to change the following for consistency.

- spark.kubernetes.operator.terminateOnInformerFailure + spark.kubernetes.operator.terminateOnInformerFailureEnabled

dongjoon-hyun · 2024-07-24T02:25:09Z

...perator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java

+        }
+      }
+      return result;
+    } catch (Exception e) {


Is this a narrowest exception?

This one was not narrowed down for metrics purpose only (so that it would be captured by the metrics). The exception is thrown at the end to ensure it's handled. There's a unit test added for this purpose as well.

dongjoon-hyun · 2024-07-24T02:26:46Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ClassLoadingUtils.java

+            listeners.add((T) listenerClass.getConstructor().newInstance());
+          }
+        }
+      } catch (Exception e) {


Is this the narrowest one?

dongjoon-hyun · 2024-07-24T02:27:39Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/MetricsSystem.java

+      sinks.forEach(Sink::stop);
+      registry.removeMatching(MetricFilter.ALL);
+    } else {
+      log.error("Stopping a MetricsSystem that is not running");


I'm wondering when you use log.isErrorEnabled? If we want to check log.isErrorEnabled, we had better use it consistently.

As recommended by PMD, we enabled according gradle task to enforce guard log statement when associate String creation and manipulation. I think this is not reported for no string manipulation

Ack. It makes sense to me too.

dongjoon-hyun · 2024-07-24T02:28:21Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/MetricsSystem.java

+  }
+
+  @Data
+  public static class SinkProps {


Can we rename SinkProps -> SinkProperties?

dongjoon-hyun · 2024-07-24T02:30:58Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java

+        && e.getCode() == 409
+        && e.getStatus() != null
+        && StringUtils.isNotEmpty(e.getStatus().toString())
+        && e.getStatus().toString().toLowerCase().contains("alreadyexists");


Just a question. When we have other string here for 409 code?

This is used to handle only specific 409 cases (e.g. request timed out but the resource was actually created, then this help the reconciler to proceed).

For other 409 conflicts - quota or status patch .etc, they are handled separately in according reconcile logic.

dongjoon-hyun · 2024-07-25T05:38:26Z

Thank you for updating.

dongjoon-hyun · 2024-07-25T05:39:26Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+  public static final ConfigOption<Double> RECONCILER_RETRY_INTERVAL_MULTIPLIER =
      ConfigOption.<Double>builder()
-          .key("spark.kubernetes.operator.retry.intervalMultiplier")
+          .key("spark.kubernetes.operator.reconciler.retry.intervalMultiplier")


Thank you for changing the namespace of retry.*.

dongjoon-hyun · 2024-07-25T05:41:20Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java

+  public static final ConfigOption<Integer> API_RETRY_MAX_ATTEMPTS =
      ConfigOption.<Integer>builder()
-          .key("spark.kubernetes.operator.retry.maxAttempts")
+          .key("spark.kubernetes.operator.api.retryMaxAttempts")


New namespace spark.kubernetes.operator.api could be misleading.

This is the backend K8s API-related configurations, right? If then, can we rename this more specifically in order to leave a room for Apache Spark Kubernetes Operator's API-related configurations?

dongjoon-hyun · 2024-07-25T05:42:17Z

spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ClassLoadingUtils.java

            listeners.add((T) listenerClass.getConstructor().newInstance());
          }
        }
-      } catch (Exception e) {


dongjoon-hyun

+1, LGTM except one comment about spark.kubernetes.operator.api

#23 (comment)

However, let's handle it seperately, @jiangzho .

I'm going to merge this in the AS-IS status.

dongjoon-hyun · 2024-07-25T05:46:41Z

Merged to main.

### What changes were proposed in this pull request? This is a breakdown PR of #12 - defines metrics system and utils classes to be used by the reconcilers. It also refactors a few methods in previous utils class `org.apache.spark.k8s.operator.reconciler.SparkReconcilerUtils` into common utils package. ### Why are the changes needed? Breakdown PRs help us to move with more flexibility. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? CIs ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#23 from jiangzho/controller_utils. Authored-by: zhou-jiang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

[SPARK-48984]Add Controller Metrics System and Utils

a6fa78b

jiangzho mentioned this pull request Jul 24, 2024

[SPARK-48382] Add reconciler to spark-operator module #12

Closed

dongjoon-hyun reviewed Jul 24, 2024

View reviewed changes

dongjoon-hyun requested changes Jul 24, 2024

View reviewed changes

Multiple style / description fix

d96b6f8

dongjoon-hyun reviewed Jul 25, 2024

View reviewed changes

dongjoon-hyun approved these changes Jul 25, 2024

View reviewed changes

dongjoon-hyun closed this in 61d013d Jul 25, 2024

dongjoon-hyun mentioned this pull request Jul 29, 2024

Operator 0.1.0 #2

Closed

[SPARK-48984]Add Controller Metrics System and Utils #23

[SPARK-48984]Add Controller Metrics System and Utils #23

Uh oh!

Conversation

jiangzho commented Jul 24, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

dongjoon-hyun 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!

jiangzho Jul 25, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 25, 2024

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!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jiangzho Jul 25, 2024 •

edited

Loading