This is an automated email from the ASF dual-hosted git repository.
fanng pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 40d54dbc2b [#8887] fix(iceberg): Return 503 when metrics endpoint
rejects requests (#8888)
40d54dbc2b is described below
commit 40d54dbc2bef1d0a81d6ebb4296a25f3488769d7
Author: Bharath Krishna <[email protected]>
AuthorDate: Thu Oct 23 01:00:42 2025 -0700
[#8887] fix(iceberg): Return 503 when metrics endpoint rejects requests
(#8888)
### What changes were proposed in this pull request?
This PR implements proper HTTP status code handling for the Iceberg
metrics endpoint as specified in the Iceberg REST API specification.
**Changes:**
1. Modified `IcebergMetricsManager.recordMetric()` to return `boolean`
instead of `void`
- Returns `true` when metric is successfully queued
- Returns `false` when manager is closed or queue is full
2. Updated `IcebergTableOperations.reportTableMetrics()` to check the
return value
- Returns HTTP 204 (No Content) when metric is accepted
- Returns HTTP 503 (Service Unavailable) when metric is rejected
3. Added unit tests in `TestIcebergMetricsManager`
### Why are the changes needed?
The Iceberg REST API specification defines specific status codes for the
metrics endpoint:
- **204 No Content**: Metric successfully accepted
- **503 Service Unavailable**: Server cannot accept metric (e.g., queue
full, service shutting down)
Previously, the endpoint always returned 204 even when metrics were
silently dropped due to queue overflow or manager shutdown. This PR
ensures clients receive proper feedback about metric acceptance.
Fix: #8887
### Does this PR introduce _any_ user-facing change?
Yes, behavioral change:
- **Before**: Metrics endpoint always returned HTTP 204, even when
metrics were dropped
- **After**: Returns HTTP 503 when metrics cannot be accepted (queue
full or manager closed)
This is backward compatible as clients should already handle 5xx
responses gracefully.
### How was this patch tested?
- Unit tests
- Validation in Dev environment:
- Tested with concurrent requests to
`/iceberg/v1/namespaces/test_db/tables/test_table/metrics` to verify it
works and not breaking.
---
.../service/metrics/IcebergMetricsManager.java | 13 +++++-
.../service/rest/IcebergTableOperations.java | 10 ++++-
.../service/metrics/TestIcebergMetricsManager.java | 48 ++++++++++++++++++++++
3 files changed, 67 insertions(+), 4 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/metrics/IcebergMetricsManager.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/metrics/IcebergMetricsManager.java
index d6ba7d6f9d..c4fc719841 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/metrics/IcebergMetricsManager.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/metrics/IcebergMetricsManager.java
@@ -108,14 +108,23 @@ public class IcebergMetricsManager {
TimeUnit.HOURS));
}
- public void recordMetric(MetricsReport metricsReport) {
+ /**
+ * Records a metrics report by adding it to the processing queue.
+ *
+ * @param metricsReport the metrics report to record
+ * @return true if the metric was successfully queued, false if it was
rejected (manager closed or
+ * queue full)
+ */
+ public boolean recordMetric(MetricsReport metricsReport) {
if (isClosed) {
logMetrics("Drop Iceberg metrics because Iceberg Metrics Manager is
closed.", metricsReport);
- return;
+ return false;
}
if (!queue.offer(metricsReport)) {
logMetrics("Drop Iceberg metrics because metrics queue is full.",
metricsReport);
+ return false;
}
+ return true;
}
public void close() {
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
index cbe0a3dc38..40b5660812 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
@@ -310,8 +310,14 @@ public class IcebergTableOperations {
return Utils.doAs(
httpRequest,
() -> {
- icebergMetricsManager.recordMetric(request.report());
- return IcebergRestUtils.noContent();
+ boolean accepted =
icebergMetricsManager.recordMetric(request.report());
+ if (accepted) {
+ return IcebergRestUtils.noContent();
+ } else {
+ return IcebergRestUtils.errorResponse(
+ new RuntimeException("Metrics service unavailable: queue
full or service closed"),
+ Response.Status.SERVICE_UNAVAILABLE.getStatusCode());
+ }
});
} catch (Exception e) {
return IcebergExceptionMapper.toRESTResponse(e);
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/metrics/TestIcebergMetricsManager.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/metrics/TestIcebergMetricsManager.java
index f02e3ecdc7..2d9cc94c76 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/metrics/TestIcebergMetricsManager.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/metrics/TestIcebergMetricsManager.java
@@ -98,4 +98,52 @@ public class TestIcebergMetricsManager {
icebergMetricsManager.close();
}
+
+ @Test
+ void testRecordMetricReturnsTrue() {
+ IcebergConfig icebergConfig = new IcebergConfig();
+ IcebergMetricsManager icebergMetricsManager = new
IcebergMetricsManager(icebergConfig);
+ icebergMetricsManager.start();
+
+ MetricsReport metricsReport = createMetricsReport();
+ boolean result = icebergMetricsManager.recordMetric(metricsReport);
+
+ Assertions.assertTrue(result, "Recording metric should return true when
successful");
+ icebergMetricsManager.close();
+ }
+
+ @Test
+ void testRecordMetricReturnsFalseWhenClosed() {
+ IcebergConfig icebergConfig = new IcebergConfig();
+ IcebergMetricsManager icebergMetricsManager = new
IcebergMetricsManager(icebergConfig);
+ icebergMetricsManager.start();
+ icebergMetricsManager.close();
+
+ MetricsReport metricsReport = createMetricsReport();
+ boolean result = icebergMetricsManager.recordMetric(metricsReport);
+
+ Assertions.assertFalse(result, "Recording metric should return false when
manager is closed");
+ }
+
+ @Test
+ void testRecordMetricReturnsFalseWhenQueueFull() {
+ Map<String, String> properties =
+ ImmutableMap.of(IcebergConstants.ICEBERG_METRICS_QUEUE_CAPACITY, "1");
+ IcebergConfig icebergConfig = new IcebergConfig(properties);
+ IcebergMetricsManager icebergMetricsManager = new
IcebergMetricsManager(icebergConfig);
+ // Don't start the manager so metrics won't be consumed from queue
+
+ MetricsReport metricsReport1 = createMetricsReport();
+ MetricsReport metricsReport2 = createMetricsReport();
+
+ // First metric should succeed
+ boolean result1 = icebergMetricsManager.recordMetric(metricsReport1);
+ Assertions.assertTrue(result1, "First metric should be queued
successfully");
+
+ // Second metric should fail because queue is full
+ boolean result2 = icebergMetricsManager.recordMetric(metricsReport2);
+ Assertions.assertFalse(result2, "Second metric should fail when queue is
full");
+
+ icebergMetricsManager.close();
+ }
}
