(spark-kubernetes-operator) branch main updated: [SPARK-49309] Use `HTTP_*` constant variables instead of magic numbers

[do...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dongjoon pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/spark-kubernetes-operator.git


The following commit(s) were added to refs/heads/main by this push:
     new f4dd102  [SPARK-49309] Use `HTTP_*` constant variables instead of magic numbers
f4dd102 is described below

commit f4dd102d91181bf171905d6a53e190d9e347876b
Author: Dongjoon Hyun <do...@apache.org>
AuthorDate: Mon Aug 19 14:24:23 2024 -0700

    [SPARK-49309] Use `HTTP_*` constant variables instead of magic numbers
    
    ### What changes were proposed in this pull request?
    
    This PR aims to use `java.net.HttpURLConnection.HTTP_*` constant variables instead of magic numbers in the code.
    
    ### Why are the changes needed?
    
    Variable names provide semantics.
    
    ```
    - if (e.getCode() == 409) {
    + if (e.getCode() == HTTP_CONFLICT) {
    ```
    
    ### Does this PR introduce _any_ user-facing change?
    
    No.
    
    ### How was this patch tested?
    
    Pass the CIs.
    
    ### Was this patch authored or co-authored using generative AI tooling?
    
    No.
    
    Closes #71 from dongjoon-hyun/SPARK-49309.
    
    Authored-by: Dongjoon Hyun <do...@apache.org>
    Signed-off-by: Dongjoon Hyun <do...@apache.org>
---
 .../spark/k8s/operator/metrics/PrometheusPullModelHandler.java   | 3 ++-
 .../java/org/apache/spark/k8s/operator/probe/HealthProbe.java    | 6 ++++--
 .../java/org/apache/spark/k8s/operator/probe/ReadinessProbe.java | 8 +++++---
 .../main/java/org/apache/spark/k8s/operator/utils/ProbeUtil.java | 9 +++++++++
 .../org/apache/spark/k8s/operator/utils/ReconcilerUtils.java     | 9 +++++----
 .../org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java | 4 +++-
 .../org/apache/spark/k8s/operator/probe/ProbeServiceTest.java    | 5 +++--
 .../org/apache/spark/k8s/operator/probe/ReadinessProbeTest.java  | 3 ++-
 8 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/PrometheusPullModelHandler.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/PrometheusPullModelHandler.java
index 73bb49b..be7b44a 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/PrometheusPullModelHandler.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/PrometheusPullModelHandler.java
@@ -19,6 +19,7 @@
 
 package org.apache.spark.k8s.operator.metrics;
 
+import static java.net.HttpURLConnection.HTTP_OK;
 import static org.apache.spark.k8s.operator.utils.ProbeUtil.sendMessage;
 
 import java.io.IOException;
@@ -57,7 +58,7 @@ public class PrometheusPullModelHandler extends PrometheusServlet implements Htt
   public void handle(HttpExchange exchange) throws IOException {
     HttpServletRequest httpServletRequest = null;
     String value = getMetricsSnapshot(httpServletRequest);
-    sendMessage(exchange, 200, String.join("\n", filterNonEmptyRecords(value)));
+    sendMessage(exchange, HTTP_OK, String.join("\n", filterNonEmptyRecords(value)));
   }
 
   protected List<String> filterNonEmptyRecords(String metricsSnapshot) {
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java
index 6e03e0e..739791a 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java
@@ -19,6 +19,8 @@
 
 package org.apache.spark.k8s.operator.probe;
 
+import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
+import static java.net.HttpURLConnection.HTTP_OK;
 import static org.apache.spark.k8s.operator.utils.ProbeUtil.areOperatorsStarted;
 import static org.apache.spark.k8s.operator.utils.ProbeUtil.sendMessage;
 
@@ -76,9 +78,9 @@ public class HealthProbe implements HttpHandler {
   @Override
   public void handle(HttpExchange exchange) throws IOException {
     if (isHealthy()) {
-      sendMessage(exchange, 200, "healthy");
+      sendMessage(exchange, HTTP_OK, "healthy");
     } else {
-      sendMessage(exchange, 500, "unhealthy");
+      sendMessage(exchange, HTTP_INTERNAL_ERROR, "unhealthy");
     }
   }
 
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/ReadinessProbe.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/ReadinessProbe.java
index be08c36..19c0cdf 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/ReadinessProbe.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/ReadinessProbe.java
@@ -19,6 +19,7 @@
 
 package org.apache.spark.k8s.operator.probe;
 
+import static java.net.HttpURLConnection.*;
 import static org.apache.spark.k8s.operator.utils.ProbeUtil.areOperatorsStarted;
 import static org.apache.spark.k8s.operator.utils.ProbeUtil.sendMessage;
 
@@ -43,14 +44,15 @@ public class ReadinessProbe implements HttpHandler {
   public void handle(HttpExchange httpExchange) throws IOException {
     Optional<Boolean> operatorsAreReady = areOperatorsStarted(operators);
     if (operatorsAreReady.isEmpty() || !operatorsAreReady.get()) {
-      sendMessage(httpExchange, 400, "spark operators are not ready yet");
+      sendMessage(httpExchange, HTTP_BAD_REQUEST, "spark operators are not ready yet");
     }
 
     if (!passRbacCheck()) {
-      sendMessage(httpExchange, 403, "required rbac test failed, operators are not ready");
+      sendMessage(
+          httpExchange, HTTP_FORBIDDEN, "required rbac test failed, operators are not ready");
     }
 
-    sendMessage(httpExchange, 200, "started");
+    sendMessage(httpExchange, HTTP_OK, "started");
   }
 
   public boolean passRbacCheck() {
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ProbeUtil.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ProbeUtil.java
index 3778f8a..5d0592e 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ProbeUtil.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ProbeUtil.java
@@ -30,11 +30,20 @@ import io.javaoperatorsdk.operator.Operator;
 import io.javaoperatorsdk.operator.RuntimeInfo;
 import lombok.extern.slf4j.Slf4j;
 
+/** A utility class to provide common functionalities for probe services. */
 @Slf4j
 public final class ProbeUtil {
 
   private ProbeUtil() {}
 
+  /**
+   * Send an HTTP response message with the given response header HTTP status code and message.
+   *
+   * @param httpExchange The handler for this HTTP response.
+   * @param code A response header HTTP status code defined in java.net.HttpURLConnection.HTTP_*
+   * @param message A message to send as a body
+   * @throws IOException Failed to send a response.
+   */
   public static void sendMessage(HttpExchange httpExchange, int code, String message)
       throws IOException {
     try (OutputStream outputStream = httpExchange.getResponseBody()) {
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java
index 39075fb..2d7bd28 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java
@@ -19,6 +19,8 @@
 
 package org.apache.spark.k8s.operator.utils;
 
+import static java.net.HttpURLConnection.HTTP_CONFLICT;
+import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
 import static org.apache.spark.k8s.operator.config.SparkOperatorConf.API_SECONDARY_RESOURCE_CREATE_MAX_ATTEMPTS;
 import static org.apache.spark.k8s.operator.config.SparkOperatorConf.RECONCILER_FOREGROUND_REQUEST_TIMEOUT_SECONDS;
 import static org.apache.spark.k8s.operator.utils.ModelUtils.buildOwnerReferenceTo;
@@ -89,8 +91,7 @@ public final class ReconcilerUtils {
                 attemptCount,
                 maxAttempts);
           }
-          // retry only on 409 Conflict
-          if (e.getCode() == 409) {
+          if (e.getCode() == HTTP_CONFLICT) {
             if (isConflictForExistingResource(e)) {
               current = getResource(client, resource);
               if (current.isPresent()) {
@@ -130,7 +131,7 @@ public final class ReconcilerUtils {
     try {
       resource = client.resource(desired).get();
     } catch (KubernetesClientException e) {
-      if (e.getCode() == 404) {
+      if (e.getCode() == HTTP_NOT_FOUND) {
         return Optional.empty();
       }
     }
@@ -150,7 +151,7 @@ public final class ReconcilerUtils {
             .delete();
       }
     } catch (KubernetesClientException e) {
-      if (e.getCode() == 404) {
+      if (e.getCode() == HTTP_NOT_FOUND) {
         log.info("Pod to delete does not exist, proceeding...");
       } else {
         throw e;
diff --git a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java
index ff7b28a..8221b4b 100644
--- a/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java
+++ b/spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/SparkExceptionUtils.java
@@ -19,6 +19,8 @@
 
 package org.apache.spark.k8s.operator.utils;
 
+import static java.net.HttpURLConnection.HTTP_CONFLICT;
+
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.exception.ExceptionUtils;
@@ -29,7 +31,7 @@ public final class SparkExceptionUtils {
 
   public static boolean isConflictForExistingResource(KubernetesClientException e) {
     return e != null
-        && e.getCode() == 409
+        && e.getCode() == HTTP_CONFLICT
         && e.getStatus() != null
         && StringUtils.isNotEmpty(e.getStatus().toString())
         && e.getStatus().toString().toLowerCase().contains("alreadyexists");
diff --git a/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ProbeServiceTest.java b/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ProbeServiceTest.java
index aeb3aa0..1e1bdd6 100644
--- a/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ProbeServiceTest.java
+++ b/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ProbeServiceTest.java
@@ -19,6 +19,7 @@
 
 package org.apache.spark.k8s.operator.probe;
 
+import static java.net.HttpURLConnection.HTTP_OK;
 import static org.apache.spark.k8s.operator.config.SparkOperatorConf.OPERATOR_PROBE_PORT;
 import static org.apache.spark.k8s.operator.probe.ProbeService.HEALTHZ;
 import static org.apache.spark.k8s.operator.probe.ProbeService.READYZ;
@@ -121,7 +122,7 @@ class ProbeServiceTest {
     HttpURLConnection connection = (HttpURLConnection) u.openConnection();
     connection.setConnectTimeout(100000);
     connection.connect();
-    assertEquals(connection.getResponseCode(), 200, "Health Probe should return 200");
+    assertEquals(connection.getResponseCode(), HTTP_OK, "Health Probe should return HTTP_OK");
   }
 
   private void hitStartedUpEndpoint() throws IOException, MalformedURLException {
@@ -129,6 +130,6 @@ class ProbeServiceTest {
     HttpURLConnection connection = (HttpURLConnection) u.openConnection();
     connection.setConnectTimeout(100000);
     connection.connect();
-    assertEquals(connection.getResponseCode(), 200, "operators are not ready");
+    assertEquals(connection.getResponseCode(), HTTP_OK, "operators are not ready");
   }
 }
diff --git a/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ReadinessProbeTest.java b/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ReadinessProbeTest.java
index 0bd4a89..e8a0850 100644
--- a/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ReadinessProbeTest.java
+++ b/spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/ReadinessProbeTest.java
@@ -19,6 +19,7 @@
 
 package org.apache.spark.k8s.operator.probe;
 
+import static java.net.HttpURLConnection.HTTP_OK;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -63,7 +64,7 @@ class ReadinessProbeTest {
     ReadinessProbe readinessProbe = new ReadinessProbe(Arrays.asList(operator));
     try (var mockedStatic = Mockito.mockStatic(ProbeUtil.class)) {
       readinessProbe.handle(httpExchange);
-      mockedStatic.verify(() -> ProbeUtil.sendMessage(httpExchange, 200, "started"));
+      mockedStatic.verify(() -> ProbeUtil.sendMessage(httpExchange, HTTP_OK, "started"));
     }
   }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org