diff --git a/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java b/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java index 252bde85a5eb..8a9ea142d72d 100644 --- a/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java +++ b/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 @ RESTCatalogAdapter.java * 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 @@ T execute( String path, Map queryParams, Object body, - Consumer errorHandler) { + HttpServletResponse response) throws IOException { ErrorResponse.Builder errorBuilder = ErrorResponse.builder(); Pair> routeAndVars = Route.from(method, path); if (routeAndVars != null) { @@ -527,63 +527,9 @@ T execute( .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 delete( - String path, - Class responseType, - Map headers, - Consumer errorHandler) { - return execute(HTTPMethod.DELETE, path, null, null, errorHandler); - } - - @Override - public T delete( - String path, - Map queryParams, - Class responseType, - Map headers, - Consumer errorHandler) { - return execute(HTTPMethod.DELETE, path, queryParams, null, errorHandler); - } - - @Override - public T post( - String path, - RESTRequest body, - Class responseType, - Map headers, - Consumer errorHandler) { - return execute(HTTPMethod.POST, path, null, body, errorHandler); - } - - @Override - public T get( - String path, - Map queryParams, - Class responseType, - Map headers, - Consumer errorHandler) { - return execute(HTTPMethod.GET, path, queryParams, null, errorHandler); - } - - @Override - public void head(String path, Map headers, Consumer errorHandler) { - execute(HTTPMethod.HEAD, path, null, headers, errorHandler); - } - - @Override - public T postForm( - String path, - Map formData, - Class responseType, - Map headers, - Consumer 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 T castResponse(Class responseType, Obj 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 pathVars) { diff --git a/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java b/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java index e608ba5229a6..21b155d65d8d 100644 --- a/standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java +++ b/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 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();