Skip to content

[WIP] HIVE-29563: Iceberg REST Catalog's error logs are too verbose #6425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
okumin wants to merge 2 commits into
base: master
Choose a base branch
from
+8 −81
Draft

[WIP] HIVE-29563: Iceberg REST Catalog's error logs are too verbose #6425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1daa058
HIVE-29563: Iceberg REST Catalog's error logs are too verbose
okumin Apr 12, 2026
9f0c70d
Remove extra abstraction
okumin Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 7 additions & 66 deletions ...store/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@
package org.apache.iceberg.rest;

import com.google.common.base.Preconditions;
import java.io.IOException;
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;
Expand All @@ -45,7 +46,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;
Expand Down Expand Up @@ -77,7 +77,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 {
private static final Splitter SLASH = Splitter.on('/');

private static final Map<Class<? extends Exception>, Integer> EXCEPTION_ERROR_CODES =
Expand Down Expand Up @@ -493,7 +493,7 @@ <T extends RESTResponse> T execute(
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) {
Expand All @@ -514,68 +514,9 @@ <T extends RESTResponse> 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 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);
}

@Override
public void close() {
// The caller is responsible for closing the underlying catalog backing this REST catalog.
response.setStatus(error.code());
RESTObjectMapper.mapper().writeValue(response.getWriter(), error);
return null;
Copy link
Copy Markdown
Contributor Author

@okumin okumin Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESTServerCatalogAdapter on apache/iceberg extends a RESTClient. I don't know the exact reason. The trick is used in some integration tests.
Anyway, as our adapter never behaves as a client, I removed the dependency on RESTClient. As a good side effect, this change ensures only a single path invokes RESTServerCatalogAdapter#execute and allows us to simplify the implementation.

}

private static class BadResponseType extends RuntimeException {
Expand Down
16 changes: 1 addition & 15 deletions ...store/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
};
}

public static class ServletRequestContext {
private HTTPMethod method;
private String path;
Expand Down
Loading