majin1102 commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r2744940597
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -273,26 +328,191 @@ public void expireDataFrom(DataExpirationConfig
expirationConfig, Instant instan
if (instant.equals(Instant.MIN)) {
return;
}
+ MaintainerMetrics metrics = context.getMetrics();
+ long startTime = System.currentTimeMillis();
+ expireDataFromWithMetrics(expirationConfig, instant, metrics, startTime);
+ }
+ private void expireDataFromWithMetrics(
+ DataExpirationConfig expirationConfig,
+ Instant instant,
+ MaintainerMetrics metrics,
+ long startTime) {
Review Comment:
I think we might need
```
if (instant.equals(Instant.MIN)) {
return;
}
```
What do you think
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -273,26 +328,191 @@ public void expireDataFrom(DataExpirationConfig
expirationConfig, Instant instan
if (instant.equals(Instant.MIN)) {
return;
}
+ MaintainerMetrics metrics = context.getMetrics();
+ long startTime = System.currentTimeMillis();
+ expireDataFromWithMetrics(expirationConfig, instant, metrics, startTime);
+ }
+ private void expireDataFromWithMetrics(
Review Comment:
I think `expireDataFrom` is just OK. It's a little werid to me to use
`FromWith`
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * 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.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.metrics.MetricDefine;
+
+/**
+ * Abstract base class for table maintenance operation metrics
+ *
+ * <p>Responsibilities:
+ *
+ * <ul>
+ * <li>Define all MetricDefine constants
+ * <li>Provide template methods for metrics registration
+ * <li>Handle tags (catalog, database, table, table_format, operation_type)
+ * </ul>
+ *
+ * <p>Note: All metrics include the table_format tag to distinguish Iceberg
and Paimon tables
+ */
+public abstract class AbstractTableMaintainerMetrics extends
AbstractTableMetrics
+ implements MaintainerMetrics {
+
+ /** Table format constant: Iceberg native table */
+ protected static final String TABLE_FORMAT_ICEBERG = "iceberg";
+
+ /** Table format constant: Paimon native table */
+ protected static final String TABLE_FORMAT_PAIMON = "paimon";
+
+ /** Table format constant: Mixed table (based on Iceberg) */
+ protected static final String TABLE_FORMAT_MIXED_ICEBERG = "mixed_iceberg";
+
+ /** Table format constant: Hive table */
+ protected static final String TABLE_FORMAT_HIVE = "hive";
+
+ // ========== Orphan Files Related MetricDefine ==========
+
+ /** Count of orphan data files cleaned */
+ public static final MetricDefine TABLE_ORPHAN_DATA_FILES_CLEANED_COUNT =
+ defineCounter("table_orphan_data_files_cleaned_count")
+ .withDescription("Count of orphan data files cleaned")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
Review Comment:
This seems a break change from `table_orphan_content_file_cleaning_count`.
This might lead to backforward compatibility issue
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -165,7 +168,17 @@ public void cleanDanglingDeleteFiles() {
Optional.ofNullable(currentSnapshot.summary().get(SnapshotSummary.TOTAL_DELETE_FILES_PROP));
if (totalDeleteFiles.isPresent() && Long.parseLong(totalDeleteFiles.get())
> 0) {
// clear dangling delete files
- doCleanDanglingDeleteFiles();
+ LOG.info("Starting cleaning dangling delete files for table {}",
table.name());
Review Comment:
So why we don't use the new interfaces and wireness?
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics
recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a
consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new
MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
Review Comment:
This class looks more like an executor that wraps maintainer operations with
metrics, rather than a classic template-method base class. To make the intent
clearer, I suggest renaming it to `MaintainerOperationExecutor` (or
MaintainerMetricsExecutor, MaintainerOperationRunner), which better reflects
that it mainly wires metrics around the operation execution.
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics
recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a
consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new
MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that
case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link
MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link
MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link
MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType,
MaintainerOperation operation)
Review Comment:
This should be a void
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * 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.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.metrics.MetricDefine;
+
+/**
+ * Abstract base class for table maintenance operation metrics
+ *
+ * <p>Responsibilities:
+ *
+ * <ul>
+ * <li>Define all MetricDefine constants
+ * <li>Provide template methods for metrics registration
+ * <li>Handle tags (catalog, database, table, table_format, operation_type)
+ * </ul>
+ *
+ * <p>Note: All metrics include the table_format tag to distinguish Iceberg
and Paimon tables
+ */
+public abstract class AbstractTableMaintainerMetrics extends
AbstractTableMetrics
Review Comment:
Wondering this abstraction is really necessary if we put that format field
into `AbstractTableMetrics`. Could you elaborate more on this?
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics
recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a
consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new
MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that
case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link
MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link
MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link
MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType,
MaintainerOperation operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return true;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording and return result.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link
MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link
MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link
MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @param <T> the result type
+ * @return the operation result
+ * @throws Throwable if the operation throws an exception
+ */
+ public <T> T executeAndReturn(
+ MaintainerOperationType operationType, MaintainerOperationWithResult<T>
operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ T result = operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return result;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /** Functional interface for maintainer operation without return value. */
+ @FunctionalInterface
+ public interface MaintainerOperation {
+ /** Executes the operation. */
+ void execute() throws Throwable;
+ }
+
+ /**
+ * Functional interface for maintainer operation with return value.
+ *
+ * @param <T> the result type
+ */
+ @FunctionalInterface
+ public interface MaintainerOperationWithResult<T> {
Review Comment:
This interface does not make sense to me
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics
recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a
consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new
MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that
case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link
MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link
MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link
MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType,
MaintainerOperation operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return true;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording and return result.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link
MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link
MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link
MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @param <T> the result type
+ * @return the operation result
+ * @throws Throwable if the operation throws an exception
+ */
+ public <T> T executeAndReturn(
+ MaintainerOperationType operationType, MaintainerOperationWithResult<T>
operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ T result = operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return result;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /** Functional interface for maintainer operation without return value. */
+ @FunctionalInterface
+ public interface MaintainerOperation {
+ /** Executes the operation. */
+ void execute() throws Throwable;
Review Comment:
I think It's a little strange to declare as `throws Throwable`.
Does any project declare like this?
And I think we could just use some interfaces JDK provides, like Runnable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
