Merge pull request #1010 from oracle/delete_retry_call

Delete superfluous ReplaceDomainWithConfictRetry

Author: rjeberhard

operator/src/main/java/oracle/kubernetes/operator/helpers/CallBuilder.java

@@ -4,8 +4,6 @@
 
 package oracle.kubernetes.operator.helpers;
 
-import static java.net.HttpURLConnection.HTTP_CONFLICT;
-
 import com.squareup.okhttp.Call;
 import io.kubernetes.client.ApiCallback;
 import io.kubernetes.client.ApiClient;
@@ -49,9 +47,6 @@
 import oracle.kubernetes.operator.calls.RequestParams;
 import oracle.kubernetes.operator.calls.SynchronousCallDispatcher;
 import oracle.kubernetes.operator.calls.SynchronousCallFactory;
-import oracle.kubernetes.operator.logging.LoggingFacade;
-import oracle.kubernetes.operator.logging.LoggingFactory;
-import oracle.kubernetes.operator.logging.MessageKeys;
 import oracle.kubernetes.operator.utils.PatchUtils;
 import oracle.kubernetes.operator.work.Step;
 import oracle.kubernetes.weblogic.domain.api.WeblogicApi;
@@ -65,8 +60,6 @@ public class CallBuilder {
   /** HTTP status code for "Not Found". */
   public static final int NOT_FOUND = 404;
 
-  private static final LoggingFacade LOGGER = LoggingFactory.getLogger("Operator", "Operator");
-
   private static final SynchronousCallDispatcher DEFAULT_DISPATCHER =
       new SynchronousCallDispatcher() {
         @Override
@@ -170,65 +163,6 @@ public VersionInfo readVersionCode() throws ApiException {
         requestParams, ((client, params) -> new VersionApi(client).getCode()));
   }
 
-  /**
-   * Class extended by callers to {@link
-   * #executeSynchronousCallWithConflictRetry(RequestParamsBuilder, SynchronousCallFactory,
-   * ConflictRetry)} for building the RequestParams to be passed to {@link
-   * #executeSynchronousCall(RequestParams, SynchronousCallFactory)}.
-   *
-   * @param <T> Type of kubernetes object to be passed to the API
-   */
-  abstract static class RequestParamsBuilder<T> {
-    T body;
-
-    public RequestParamsBuilder(T body) {
-      this.body = body;
-    }
-
-    abstract RequestParams buildRequestParams();
-
-    void setBody(T body) {
-      this.body = body;
-    }
-  }
-
-  private <T> T executeSynchronousCallWithConflictRetry(
-      RequestParamsBuilder requestParamsBuilder,
-      SynchronousCallFactory<T> factory,
-      ConflictRetry<T> conflictRetry)
-      throws ApiException {
-    int retryCount = 0;
-    while (retryCount == 0 || retryCount < maxRetryCount) {
-      retryCount++;
-      RequestParams requestParams = requestParamsBuilder.buildRequestParams();
-      try {
-        return executeSynchronousCall(requestParams, factory);
-      } catch (ApiException apiException) {
-        boolean retry = false;
-        if (apiException.getCode() == HTTP_CONFLICT
-            && conflictRetry != null
-            && retryCount < maxRetryCount) {
-          T body = conflictRetry.getUpdatedObject();
-          if (body != null) {
-            requestParamsBuilder.setBody(body);
-            retry = true;
-            LOGGER.fine(
-                MessageKeys.SYNC_RETRY,
-                requestParams.call,
-                apiException.getCode(),
-                apiException.getMessage(),
-                retryCount,
-                maxRetryCount);
-          }
-        }
-        if (!retry) {
-          throw apiException;
-        }
-      }
-    }
-    return null;
-  }
-
   private <T> T executeSynchronousCall(
       RequestParams requestParams, SynchronousCallFactory<T> factory) throws ApiException {
     return DISPATCHER.execute(factory, requestParams, helper);
@@ -365,33 +299,6 @@ public Step readDomainAsync(String name, String namespace, ResponseStep<Domain>
                   pretty,
                   null);
 
-  /**
-   * Replace domain.
-   *
-   * @param uid the domain uid (unique within the k8s cluster)
-   * @param namespace Namespace
-   * @param body Body
-   * @param conflictRetry ConflictRetry implementation to be called to obtain the latest version of
-   *     the Domain for retrying the replaceDomain synchronous call if previous call failed with
-   *     Conflict response code (409)
-   * @return Replaced domain
-   * @throws ApiException APIException
-   */
-  public Domain replaceDomainWithConflictRetry(
-      String uid, String namespace, Domain body, ConflictRetry<Domain> conflictRetry)
-      throws ApiException {
-    return executeSynchronousCallWithConflictRetry(
-        new RequestParamsBuilder<Domain>(body) {
-
-          @Override
-          RequestParams buildRequestParams() {
-            return new RequestParams("replaceDomain", namespace, uid, body);
-          }
-        },
-        REPLACE_DOMAIN_CALL,
-        conflictRetry);
-  }
-
   /**
    * Replace domain.
    *

operator/src/test/java/oracle/kubernetes/operator/helpers/CallBuilderTest.java

@@ -6,11 +6,9 @@
 
 import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
 import static java.net.HttpURLConnection.HTTP_CONFLICT;
-import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.junit.MatcherAssert.assertThat;
-import static org.junit.Assert.fail;
 
 import com.google.gson.GsonBuilder;
 import com.meterware.pseudoserver.HttpUserAgentTest;
@@ -29,7 +27,6 @@
 import io.kubernetes.client.models.V1Status;
 import io.kubernetes.client.models.VersionInfo;
 import java.io.IOException;
-import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -70,7 +67,7 @@ public void setUp() throws NoSuchFieldException {
   }
 
   @After
-  public void tearDown() throws Exception {
+  public void tearDown() {
     for (Memento memento : mementos) memento.revert();
   }
 
@@ -117,133 +114,6 @@ public void replaceDomain_conflictResponseCode_throws() throws ApiException {
     callBuilder.replaceDomain(UID, NAMESPACE, domain);
   }
 
-  @Test
-  public void replaceDomainWithRetry_sendsNewDomain() throws ApiException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    defineHttpPutResponse(
-        DOMAIN_RESOURCE, UID, domain, (json) -> requestBody = fromJson(json, Domain.class));
-
-    callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-
-    assertThat(requestBody, equalTo(domain));
-  }
-
-  @Test
-  public void replaceDomainWithRetry_withMaxRetryCountOfZero_sendsNewDomain()
-      throws ApiException, NoSuchFieldException, IllegalAccessException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    defineHttpPutResponse(
-        DOMAIN_RESOURCE, UID, domain, (json) -> requestBody = fromJson(json, Domain.class));
-
-    setMaxRetryCount(callBuilder, 0);
-    callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-
-    assertThat(requestBody, equalTo(domain));
-  }
-
-  @Test
-  public void replaceDomainWithRetry_sendsNewDomain_afterRetry() throws ApiException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    ConflictOncePutServlet conflictOncePutServlet =
-        new ConflictOncePutServlet(domain, (json) -> requestBody = fromJson(json, Domain.class));
-    defineResource(DOMAIN_RESOURCE + "/" + UID, conflictOncePutServlet);
-
-    callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-
-    assertThat(requestBody, equalTo(domain));
-    assertThat(conflictOncePutServlet.conflictReturned, equalTo(true));
-  }
-
-  @Test(expected = ApiException.class)
-  public void replaceDomainWithConflictRetry_conflictResponseCode_throws() throws ApiException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, new ErrorCodePutServlet(HTTP_CONFLICT));
-
-    callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-  }
-
-  @Test(expected = ApiException.class)
-  public void replaceDomainWithConflictRetry_errorResponseCode_throws() throws ApiException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, new ErrorCodePutServlet(HTTP_BAD_REQUEST));
-
-    callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-  }
-
-  @Test
-  public void replaceDomainWithConflictRetry_conflictResponseCode_retriedMaxTimes()
-      throws ApiException, NoSuchFieldException, IllegalAccessException {
-    final int MAX_RETRY_COUNT = 5;
-    setMaxRetryCount(callBuilder, MAX_RETRY_COUNT);
-    Domain domain = new Domain().withMetadata(createMetadata());
-    ErrorCodePutServlet conflictPutServlet = new ErrorCodePutServlet(HTTP_CONFLICT);
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, conflictPutServlet);
-    try {
-      callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-      fail("Expected ApiException not thrown");
-    } catch (ApiException apiException) {
-
-    }
-    assertThat(conflictPutServlet.numGetPutResponseCalled, equalTo(MAX_RETRY_COUNT));
-  }
-
-  @Test
-  public void replaceDomainWithConflictRetry_otherResponseCode_noRetries()
-      throws ApiException, NoSuchFieldException, IllegalAccessException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    ErrorCodePutServlet conflictPutServlet = new ErrorCodePutServlet(HTTP_INTERNAL_ERROR);
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, conflictPutServlet);
-    try {
-      callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-      fail("Expected ApiException not thrown");
-    } catch (ApiException apiException) {
-
-    }
-    assertThat(conflictPutServlet.numGetPutResponseCalled, equalTo(1));
-  }
-
-  @Test
-  public void replaceDomainWithConflictRetry_withMaxRetryCountOfZero_noRetries()
-      throws ApiException, NoSuchFieldException, IllegalAccessException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    ErrorCodePutServlet conflictPutServlet = new ErrorCodePutServlet(HTTP_CONFLICT);
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, conflictPutServlet);
-    setMaxRetryCount(callBuilder, 0);
-    try {
-      callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> domain);
-      fail("Expected ApiException not thrown");
-    } catch (ApiException apiException) {
-      assertThat(apiException.getCode(), equalTo(HTTP_CONFLICT));
-    }
-    assertThat(conflictPutServlet.numGetPutResponseCalled, equalTo(1));
-  }
-
-  @Test
-  public void replaceDomainWithConflictRetry_nullUpdatedObject_noRetries()
-      throws ApiException, NoSuchFieldException, IllegalAccessException {
-    Domain domain = new Domain().withMetadata(createMetadata());
-    ErrorCodePutServlet conflictPutServlet = new ErrorCodePutServlet(HTTP_CONFLICT);
-    defineHttpPutResponse(DOMAIN_RESOURCE, UID, domain, conflictPutServlet);
-    try {
-      callBuilder.replaceDomainWithConflictRetry(UID, NAMESPACE, domain, () -> null);
-      fail("Expected ApiException not thrown");
-    } catch (ApiException apiException) {
-      assertThat(apiException.getCode(), equalTo(HTTP_CONFLICT));
-    }
-    assertThat(conflictPutServlet.numGetPutResponseCalled, equalTo(1));
-  }
-
-  Field callBuilderMaxRetryCount;
-
-  private void setMaxRetryCount(CallBuilder callBuilder, int maxRetryCount)
-      throws IllegalAccessException, NoSuchFieldException {
-    if (callBuilderMaxRetryCount == null) {
-      callBuilderMaxRetryCount = CallBuilder.class.getDeclaredField("maxRetryCount");
-      callBuilderMaxRetryCount.setAccessible(true);
-    }
-    callBuilderMaxRetryCount.set(callBuilder, maxRetryCount);
-  }
-
   @Test
   public void createPV_returnsVolumeAsJson() throws ApiException {
     V1PersistentVolume volume = createPersistentVolume();
@@ -341,6 +211,7 @@ private void defineHttpPutResponse(
     defineResource(resourceName + "/" + name, new JsonPutServlet(response, bodyValidation));
   }
 
+  @SuppressWarnings("unused")
   private void defineHttpPutResponse(
       String resourceName, String name, Object response, PseudoServlet pseudoServlet) {
     defineResource(resourceName + "/" + name, pseudoServlet);
@@ -395,7 +266,7 @@ static class ErrorCodePutServlet extends PseudoServlet {
     int numGetPutResponseCalled = 0;
     final int errorCode;
 
-    public ErrorCodePutServlet(int errorCode) {
+    ErrorCodePutServlet(int errorCode) {
       this.errorCode = errorCode;
     }
 
@@ -406,24 +277,6 @@ public WebResource getPutResponse() {
     }
   }
 
-  static class ConflictOncePutServlet extends JsonBodyServlet {
-
-    boolean conflictReturned;
-
-    private ConflictOncePutServlet(Object returnValue, Consumer<String> bodyValidation) {
-      super(returnValue, bodyValidation);
-    }
-
-    @Override
-    public WebResource getPutResponse() throws IOException {
-      if (!conflictReturned) {
-        conflictReturned = true;
-        return new WebResource("", HTTP_CONFLICT);
-      }
-      return getResponse();
-    }
-  }
-
   abstract static class JsonServlet extends PseudoServlet {
 
     private WebResource response;
@@ -448,6 +301,7 @@ private void validateParameters() throws IOException {
       if (!validationErrors.isEmpty()) throw new IOException(String.join("\n", validationErrors));
     }
 
+    @SuppressWarnings("UnusedReturnValue")
     JsonServlet expectingParameter(String name, String value) {
       parameterExpectations.add(new ParameterExpectation(name, value));
       return this;
@@ -516,10 +370,6 @@ public WebResource getPostResponse() throws IOException {
 
   static class JsonPutServlet extends JsonBodyServlet {
 
-    private JsonPutServlet(Object returnValue) {
-      this(returnValue, null);
-    }
-
     private JsonPutServlet(Object returnValue, Consumer<String> bodyValidation) {
       super(returnValue, bodyValidation);
     }