HIVE-29563: Iceberg REST Catalog's error logs are too verbose

standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java

@@ -21,12 +21,13 @@
 
 import com.google.common.base.Preconditions;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.time.Clock;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
-import java.util.function.Consumer;
+import javax.servlet.http.HttpServletResponse;
 import org.apache.iceberg.BaseTable;
 import org.apache.iceberg.BaseTransaction;
 import org.apache.iceberg.Table;
@@ -48,7 +49,6 @@
 import org.apache.iceberg.exceptions.NoSuchTableException;
 import org.apache.iceberg.exceptions.NoSuchViewException;
 import org.apache.iceberg.exceptions.NotAuthorizedException;
-import org.apache.iceberg.exceptions.RESTException;
 import org.apache.iceberg.exceptions.UnprocessableEntityException;
 import org.apache.iceberg.exceptions.ValidationException;
 import org.apache.iceberg.relocated.com.google.common.base.Splitter;
@@ -83,7 +83,7 @@
  * Original @ <a href="https://github.com/apache/iceberg/blob/apache-iceberg-1.9.1/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java">RESTCatalogAdapter.java</a>
  * Adaptor class to translate REST requests into {@link Catalog} API calls.
  */
-public class HMSCatalogAdapter implements RESTClient {
+public class HMSCatalogAdapter implements Closeable {
   private static final Logger LOG = LoggerFactory.getLogger(HMSCatalogAdapter.class);
   private static final Splitter SLASH = Splitter.on('/');
 
@@ -506,7 +506,7 @@
       String path,
       Map<String, String> queryParams,
       Object body,
-      Consumer<ErrorResponse> errorHandler) {
+      HttpServletResponse response) throws IOException {
     ErrorResponse.Builder errorBuilder = ErrorResponse.builder();
     Pair<Route, Map<String, String>> routeAndVars = Route.from(method, path);
     if (routeAndVars != null) {
@@ -527,63 +527,9 @@
           .withMessage(String.format("No route for request: %s %s", method, path));
     }
     ErrorResponse error = errorBuilder.build();
-    errorHandler.accept(error);
-    // if the error handler doesn't throw an exception, throw a generic one
-    throw new RESTException("Unhandled error: %s", error);
-  }
-
-  @Override
-  public <T extends RESTResponse> T delete(
-      String path,
-      Class<T> responseType,
-      Map<String, String> headers,
-      Consumer<ErrorResponse> errorHandler) {
-    return execute(HTTPMethod.DELETE, path, null, null, errorHandler);
-  }
-
-  @Override
-  public <T extends RESTResponse> T delete(
-      String path,
-      Map<String, String> queryParams,
-      Class<T> responseType,
-      Map<String, String> headers,
-      Consumer<ErrorResponse> errorHandler) {
-    return execute(HTTPMethod.DELETE, path, queryParams, null, errorHandler);
-  }
-
-  @Override
-  public <T extends RESTResponse> T post(
-      String path,
-      RESTRequest body,
-      Class<T> responseType,
-      Map<String, String> headers,
-      Consumer<ErrorResponse> errorHandler) {
-    return execute(HTTPMethod.POST, path, null, body, errorHandler);
-  }
-
-  @Override
-  public <T extends RESTResponse> T get(
-      String path,
-      Map<String, String> queryParams,
-      Class<T> responseType,
-      Map<String, String> headers,
-      Consumer<ErrorResponse> errorHandler) {
-    return execute(HTTPMethod.GET, path, queryParams, null, errorHandler);
-  }
-
-  @Override
-  public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
-    execute(HTTPMethod.HEAD, path, null, headers, errorHandler);
-  }
-
-  @Override
-  public <T extends RESTResponse> T postForm(
-      String path,
-      Map<String, String> formData,
-      Class<T> responseType,
-      Map<String, String> headers,
-      Consumer<ErrorResponse> errorHandler) {
-    return execute(HTTPMethod.POST, path, null, formData, errorHandler);
+    response.setStatus(error.code());
+    RESTObjectMapper.mapper().writeValue(response.getWriter(), error);
+    return null;
   }
 
   @Override
@@ -627,11 +573,15 @@
 
   public static void configureResponseFromException(
       Exception exc, ErrorResponse.Builder errorBuilder) {
-    errorBuilder
-        .responseCode(EXCEPTION_ERROR_CODES.getOrDefault(exc.getClass(), 500))
-        .withType(exc.getClass().getSimpleName())
-        .withMessage(exc.getMessage())
-        .withStackTrace(exc);
+    var errorCode = EXCEPTION_ERROR_CODES.getOrDefault(exc.getClass(), 500);
+    errorBuilder.responseCode(errorCode).withType(exc.getClass().getSimpleName()).withMessage(exc.getMessage());
+    // avoid exposing stack traces for client errors but include them for server errors to aid debugging
+    if (errorCode >= 500) {
+      LOG.error("A server error happened while processing REST request", exc);
+      errorBuilder.withStackTrace(exc);
+    } else {
+      LOG.info("A client error happened while processing REST request: {}", exc.getMessage());
+    }
   }
 
   private static Namespace namespaceFromPathVars(Map<String, String> pathVars) {

standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java

@@ -20,10 +20,8 @@
 package org.apache.iceberg.rest;
 
 import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.util.Map;
 import java.util.Optional;
-import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
@@ -77,29 +75,17 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
               context.path(),
               context.queryParams(),
               context.body(),
-              handle(response));
+              response);
 
       if (responseBody != null) {
         RESTObjectMapper.mapper().writeValue(response.getWriter(), responseBody);
       }
     } catch (RuntimeException | IOException e) {
-      // should be a RESTException but not able to see them through dependencies
       LOG.error("Error processing REST request", e);
       response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
     }
   }
 
-  private Consumer<ErrorResponse> handle(HttpServletResponse response) {
-    return errorResponse -> {
-      response.setStatus(errorResponse.code());
-      try {
-        RESTObjectMapper.mapper().writeValue(response.getWriter(), errorResponse);
-      } catch (IOException e) {
-        throw new UncheckedIOException(e);
-      }
-    };
-  }
-
   @Override
   public void destroy() {
     super.destroy();