-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29593: Server-side Metrics Reporting for Iceberg operations #6461
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.
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
Changes from all commits
1e82794
cba14c8
49704bf
d896dfa
69c2efd
e7941bc
60bbbc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,12 @@ private Consumer<ErrorResponse> handle(HttpServletResponse response) { | |
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public void destroy() { | ||
| super.destroy(); | ||
| restCatalogAdapter.close(); | ||
| } | ||
|
|
||
| public static class ServletRequestContext { | ||
| private HTTPMethod method; | ||
| private String path; | ||
|
|
@@ -147,8 +153,15 @@ static ServletRequestContext from(HttpServletRequest request) throws IOException | |
| Route route = routeContext.first(); | ||
| Object requestBody = null; | ||
| if (route.requestClass() != null) { | ||
| requestBody = | ||
| RESTObjectMapper.mapper().readValue(request.getReader(), route.requestClass()); | ||
| try { | ||
| requestBody = RESTObjectMapper.mapper().readValue(request.getReader(), route.requestClass()); | ||
| } catch (Exception e) { | ||
| return new ServletRequestContext(ErrorResponse.builder() | ||
| .responseCode(400) | ||
| .withType(e.getClass().getSimpleName()) | ||
| .withMessage(e.getMessage()) | ||
| .build()); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I happened to find an invalid body throws a 500 error |
||
| } | ||
| } | ||
|
|
||
| Map<String, String> queryParams = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.iceberg.rest.metrics; | ||
|
|
||
| import org.apache.iceberg.catalog.TableIdentifier; | ||
| import org.apache.iceberg.metrics.MetricsReport; | ||
|
|
||
| import java.io.Closeable; | ||
| import java.time.Instant; | ||
|
|
||
| /** | ||
| * An event reporter for Iceberg metrics. | ||
| */ | ||
| public interface IcebergMetricsReporter extends Closeable { | ||
| /** | ||
| * Report an event. | ||
| * | ||
| * @param catalog the catalog name | ||
| * @param identifier the table identifier | ||
| * @param report the event | ||
| * @param receivedAt the timestamp when the Iceberg REST Catalog received the event | ||
| */ | ||
| void report(String catalog, TableIdentifier identifier, MetricsReport report, Instant receivedAt); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: Polaris and Gravitino's SPI
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.iceberg.rest.metrics; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.iceberg.catalog.TableIdentifier; | ||
| import org.apache.iceberg.metrics.MetricsReport; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.time.Instant; | ||
|
|
||
| /** | ||
| * A metrics reporter that logs events. | ||
| */ | ||
| public class LoggingMetricsReporter implements IcebergMetricsReporter { | ||
| private static final Logger LOG = LoggerFactory.getLogger(LoggingMetricsReporter.class); | ||
|
|
||
| public LoggingMetricsReporter(Configuration conf) { | ||
| } | ||
|
|
||
| @Override | ||
| public void report(String catalog, TableIdentifier identifier, MetricsReport report, Instant receivedAt) { | ||
| LOG.info("Event reported at {}: catalog={}, table={}, report={}", receivedAt, catalog, identifier, report); | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| LOG.info("Closing {}", getClass().getName()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| package org.apache.iceberg.rest.extension; | ||
|
|
||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.UUID; | ||
| import org.apache.hadoop.conf.Configuration; | ||
|
|
@@ -60,6 +61,9 @@ public void start(Configuration conf) throws Exception { | |
|
|
||
| String uniqueTestKey = String.format("RESTCatalogServer_%s", UUID.randomUUID()); | ||
| warehouseDir = Path.of(MetaStoreTestUtils.getTestWarehouseDir(uniqueTestKey)); | ||
| Files.createDirectories(warehouseDir); | ||
| System.setProperty("derby.stream.error.file", | ||
| warehouseDir.resolve("derby.log").toAbsolutePath().toString()); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log was generated in |
||
| String jdbcUrl = String.format("jdbc:derby:memory:%s;create=true", | ||
| warehouseDir.resolve("metastore_db").toAbsolutePath()); | ||
| MetastoreConf.setVar(conf, ConfVars.CONNECT_URL_KEY, jdbcUrl); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case if multiple reported are present and there is exception in closing 1 reporter, rest all will not be closed. Can we do something like this ?
hive/ql/src/java/org/apache/hadoop/hive/ql/util/CloseableThreadLocal.java
Lines 48 to 58 in cff3c62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I found the close method would not be invoked, so I added a Servlet lifecycle method and logging for safety.
69c2efd