Skip to content

Commit bd9a4b6

Browse files
terrytluterrytlu
committed
[AMORO-4292] Guard IcebergTableMaintainer orphan cleanup against shared table locations, add metrics monitoring orphan file cleaning status
Introduce last-value gauges to track orphan-file-cleaning outcomes: - table_orphan_file_cleaning_last_status (0=SUCCESS, 1=CONFLICT_DETECTED, 2=CHECK_UNAVAILABLE, 3=EXECUTION_FAILED) - table_orphan_file_cleaning_last_failure_timestamp_ms MaintainerMetrics gains CleanFailureReason and recordSuccess/recordFailure. IcebergTableMaintainer wraps cleanup in try/catch and records success/failure, with location-conflict check returning CONFLICT_DETECTED/CHECK_FAILED.
1 parent 090cee1 commit bd9a4b6

6 files changed

Lines changed: 301 additions & 56 deletions

File tree

‎amoro-ams/src/main/java/org/apache/amoro/server/optimizing/maintainer/DefaultTableMaintainerContext.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,16 @@ public void recordOrphanDataFilesCleaned(int expected, int cleaned) {
6767
public void recordOrphanMetadataFilesCleaned(int expected, int cleaned) {
6868
metrics.completeOrphanMetadataFiles(expected, cleaned);
6969
}
70+
71+
@Override
72+
public void recordSuccess() {
73+
metrics.recordSuccess();
74+
}
75+
76+
@Override
77+
public void recordFailure(MaintainerMetrics.CleanFailureReason reason) {
78+
metrics.recordFailure(reason);
79+
}
7080
};
7181
}
7282

‎amoro-ams/src/main/java/org/apache/amoro/server/table/TableOrphanFilesCleaningMetrics.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,13 @@
1919
package org.apache.amoro.server.table;
2020

2121
import static org.apache.amoro.metrics.MetricDefine.defineCounter;
22+
import static org.apache.amoro.metrics.MetricDefine.defineGauge;
2223

2324
import org.apache.amoro.ServerTableIdentifier;
2425
import org.apache.amoro.maintainer.MaintainerMetrics;
26+
import org.apache.amoro.maintainer.MaintainerMetrics.CleanFailureReason;
2527
import org.apache.amoro.metrics.Counter;
28+
import org.apache.amoro.metrics.Gauge;
2629
import org.apache.amoro.metrics.MetricDefine;
2730
import org.apache.amoro.metrics.MetricRegistry;
2831

@@ -35,6 +38,13 @@ public class TableOrphanFilesCleaningMetrics extends AbstractTableMetrics
3538
private final Counter orphanMetadataFilesCount = new Counter();
3639
private final Counter expectedOrphanMetadataFilesCount = new Counter();
3740

41+
// ---- last-value gauges ----
42+
private volatile int lastStatus = STATUS_SUCCESS;
43+
private volatile long lastFailureTimestampMs = 0L;
44+
45+
// --- status constants ---
46+
public static final int STATUS_SUCCESS = 0;
47+
3848
public TableOrphanFilesCleaningMetrics(ServerTableIdentifier identifier) {
3949
super(identifier);
4050
}
@@ -65,6 +75,24 @@ public TableOrphanFilesCleaningMetrics(ServerTableIdentifier identifier) {
6575
.withTags("catalog", "database", "table")
6676
.build();
6777

78+
// ---- new orphan-file-cleaning status metrics ----
79+
80+
public static final MetricDefine TABLE_ORPHAN_FILE_CLEANING_LAST_STATUS =
81+
defineGauge("table_orphan_file_cleaning_last_status")
82+
.withDescription(
83+
"Status of the most recent orphan-file-cleaning attempt; "
84+
+ "see MaintainerMetrics.CleanFailureReason: "
85+
+ "0=SUCCESS, 1=LOCATION_CONFLICT, 2=LOCATION_CONFLICT_CHECK_FAILED, "
86+
+ "3=EXECUTION_FAILED")
87+
.withTags("catalog", "database", "table")
88+
.build();
89+
90+
public static final MetricDefine TABLE_ORPHAN_FILE_CLEANING_LAST_FAILURE_TIMESTAMP_MS =
91+
defineGauge("table_orphan_file_cleaning_last_failure_timestamp_ms")
92+
.withDescription("Epoch millis of the last real orphan-file-cleaning failure")
93+
.withTags("catalog", "database", "table")
94+
.build();
95+
6896
@Override
6997
public void registerMetrics(MetricRegistry registry) {
7098
if (globalRegistry == null) {
@@ -78,6 +106,15 @@ public void registerMetrics(MetricRegistry registry) {
78106
registry,
79107
TABLE_EXPECTED_ORPHAN_METADATA_FILE_CLEANING_COUNT,
80108
expectedOrphanMetadataFilesCount);
109+
110+
// new gauges
111+
registerMetric(
112+
registry, TABLE_ORPHAN_FILE_CLEANING_LAST_STATUS, (Gauge<Integer>) () -> lastStatus);
113+
registerMetric(
114+
registry,
115+
TABLE_ORPHAN_FILE_CLEANING_LAST_FAILURE_TIMESTAMP_MS,
116+
(Gauge<Long>) () -> lastFailureTimestampMs);
117+
81118
globalRegistry = registry;
82119
}
83120
}
@@ -101,4 +138,40 @@ public void recordOrphanDataFilesCleaned(int expected, int cleaned) {
101138
public void recordOrphanMetadataFilesCleaned(int expected, int cleaned) {
102139
completeOrphanMetadataFiles(expected, cleaned);
103140
}
141+
142+
// ---- public mutation API ----
143+
144+
/**
145+
* Record a successful orphan-file-cleaning run. Resets {@code last_status} to {@link
146+
* #STATUS_SUCCESS}.
147+
*
148+
* <p>Note: {@code lastFailureTimestampMs} is intentionally preserved across success runs. It
149+
* tracks the time of the most recent real failure and is needed by monitoring to alert on
150+
* stale-failure windows. Only an actual failure (via {@link #recordFailure(CleanFailureReason)})
151+
* refreshes it; a success run is independent.
152+
*/
153+
@Override
154+
public void recordSuccess() {
155+
this.lastStatus = STATUS_SUCCESS;
156+
}
157+
158+
/**
159+
* Record a failure event. Updates {@code last_status} to the failure reason and refreshes {@code
160+
* lastFailureTimestampMs}.
161+
*/
162+
@Override
163+
public void recordFailure(CleanFailureReason reason) {
164+
this.lastStatus = reason.statusCode();
165+
this.lastFailureTimestampMs = System.currentTimeMillis();
166+
}
167+
168+
// ---- package-private / test-visible accessors ----
169+
170+
int getLastStatus() {
171+
return lastStatus;
172+
}
173+
174+
long getLastFailureTimestampMs() {
175+
return lastFailureTimestampMs;
176+
}
104177
}

‎amoro-ams/src/test/java/org/apache/amoro/server/table/TestTableOrphanFilesCleaningMetrics.java‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.amoro.server.table;
20+
21+
import org.apache.amoro.ServerTableIdentifier;
22+
import org.apache.amoro.TableFormat;
23+
import org.apache.amoro.metrics.MetricRegistry;
24+
import org.apache.amoro.maintainer.MaintainerMetrics.CleanFailureReason;
25+
import org.junit.After;
26+
import org.junit.Before;
27+
import org.junit.Test;
28+
29+
import static org.junit.Assert.assertEquals;
30+
import static org.junit.Assert.assertTrue;
31+
32+
/**
33+
* Unit tests for {@link TableOrphanFilesCleaningMetrics} — the 2 last-value gauges introduced for
34+
* orphan-file-cleaning monitoring. These tests exercise the {@code recordSuccess()} and {@code
35+
* recordFailure(reason)} methods directly against the metric objects registered in a {@link
36+
* MetricRegistry}, verifying the Gauge <em>last-value</em> semantics.
37+
*/
38+
public class TestTableOrphanFilesCleaningMetrics {
39+
40+
private MetricRegistry registry;
41+
private TableOrphanFilesCleaningMetrics metrics;
42+
43+
@Before
44+
public void setUp() {
45+
registry = new MetricRegistry();
46+
metrics =
47+
new TableOrphanFilesCleaningMetrics(
48+
ServerTableIdentifier.of("test_catalog", "test_db", "test_table", TableFormat.ICEBERG));
49+
metrics.register(registry);
50+
}
51+
52+
@After
53+
public void tearDown() {
54+
if (metrics != null) {
55+
metrics.unregister();
56+
}
57+
}
58+
59+
// ---- baseline initialization ----
60+
61+
@Test
62+
public void testInitialStateIsSuccess() {
63+
assertEquals(TableOrphanFilesCleaningMetrics.STATUS_SUCCESS, metrics.getLastStatus());
64+
assertEquals(0L, metrics.getLastFailureTimestampMs());
65+
}
66+
67+
// ---- recordSuccess resets last_status but preserves lastFailureTimestampMs ----
68+
69+
@Test
70+
public void testRecordSuccessAfterFailureResetsStatus() {
71+
metrics.recordFailure(CleanFailureReason.LOCATION_CONFLICT);
72+
assertEquals(CleanFailureReason.LOCATION_CONFLICT.statusCode(), metrics.getLastStatus());
73+
long failureTsBefore = metrics.getLastFailureTimestampMs();
74+
assertTrue("lastFailureTimestampMs should be > 0 after a real failure", failureTsBefore > 0);
75+
76+
metrics.recordSuccess();
77+
assertEquals(TableOrphanFilesCleaningMetrics.STATUS_SUCCESS, metrics.getLastStatus());
78+
assertEquals(
79+
"recordSuccess must NOT reset lastFailureTimestampMs — timestamp persists so monitoring"
80+
+ " can alert on stale-failure windows",
81+
failureTsBefore,
82+
metrics.getLastFailureTimestampMs());
83+
}
84+
85+
// ---- recordFailure for each "real failure" reason updates last_status / ts ----
86+
87+
@Test
88+
public void testLocationConflictUpdatesLastStatusAndTs() {
89+
metrics.recordFailure(CleanFailureReason.LOCATION_CONFLICT);
90+
assertEquals(CleanFailureReason.LOCATION_CONFLICT.statusCode(), metrics.getLastStatus());
91+
assertTrue(
92+
"lastFailureTimestampMs should be > 0 on a real failure",
93+
metrics.getLastFailureTimestampMs() > 0);
94+
}
95+
96+
@Test
97+
public void testLocationCheckUnavailableUpdatesLastStatusAndTs() {
98+
metrics.recordFailure(CleanFailureReason.LOCATION_CONFLICT_CHECK_FAILED);
99+
assertEquals(
100+
CleanFailureReason.LOCATION_CONFLICT_CHECK_FAILED.statusCode(), metrics.getLastStatus());
101+
assertTrue(metrics.getLastFailureTimestampMs() > 0);
102+
}
103+
104+
@Test
105+
public void testExecutionFailedUpdatesLastStatusAndTs() {
106+
metrics.recordFailure(CleanFailureReason.EXECUTION_FAILED);
107+
assertEquals(CleanFailureReason.EXECUTION_FAILED.statusCode(), metrics.getLastStatus());
108+
assertTrue(metrics.getLastFailureTimestampMs() > 0);
109+
}
110+
}

‎amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerMetrics.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,36 @@ public interface MaintainerMetrics {
4040
*/
4141
void recordOrphanMetadataFilesCleaned(int expected, int cleaned);
4242

43+
/** Record a successful orphan-file-cleaning run. */
44+
void recordSuccess();
45+
46+
/**
47+
* Record an orphan-file-cleaning failure event.
48+
*
49+
* @param reason the failure reason
50+
*/
51+
void recordFailure(CleanFailureReason reason);
52+
53+
/** Atomic failure reasons for orphan-file-cleaning observability. */
54+
enum CleanFailureReason {
55+
/** Another table uuid detected in the same location; cleanup skipped. */
56+
LOCATION_CONFLICT(1),
57+
/** The location-conflict check itself failed (e.g. FileIO doesn't support prefix ops). */
58+
LOCATION_CONFLICT_CHECK_FAILED(2),
59+
/** The actual cleaning execution threw an exception. */
60+
EXECUTION_FAILED(3);
61+
62+
private final int statusCode;
63+
64+
CleanFailureReason(int statusCode) {
65+
this.statusCode = statusCode;
66+
}
67+
68+
public int statusCode() {
69+
return statusCode;
70+
}
71+
}
72+
4373
/** No-op implementation that does nothing. */
4474
MaintainerMetrics NOOP =
4575
new MaintainerMetrics() {
@@ -48,5 +78,11 @@ public void recordOrphanDataFilesCleaned(int expected, int cleaned) {}
4878

4979
@Override
5080
public void recordOrphanMetadataFilesCleaned(int expected, int cleaned) {}
81+
82+
@Override
83+
public void recordSuccess() {}
84+
85+
@Override
86+
public void recordFailure(CleanFailureReason reason) {}
5187
};
5288
}

0 commit comments

Comments
 (0)