Copilot commented on code in PR #10138:
URL: https://github.com/apache/gravitino/pull/10138#discussion_r2877794889
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/api/monitor/MetricsProvider.java:
##########
@@ -49,8 +47,7 @@ Map<String, List<MetricSample>> jobMetrics(
* @param endTime end timestamp (seconds)
* @return map keyed by metric name, each containing a metric sample series
*/
- Map<String, List<MetricSample>> tableMetrics(
- NameIdentifier tableIdentifier, long startTime, long endTime);
+ List<MetricPoint> tableMetrics(NameIdentifier tableIdentifier, long
startTime, long endTime);
Review Comment:
The JavaDoc for this method still mentions returning a "map keyed by metric
name", but the method now returns a List<MetricPoint>. Please align the
`@return` description with the updated API.
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/api/monitor/MetricsProvider.java:
##########
@@ -61,6 +58,6 @@ Map<String, List<MetricSample>> tableMetrics(
* @param endTime end timestamp (seconds)
* @return map keyed by metric name, each containing a metric sample series
*/
- Map<String, List<MetricSample>> partitionMetrics(
+ List<MetricPoint> partitionMetrics(
NameIdentifier tableIdentifier, PartitionPath partitionPath, long
startTime, long endTime);
Review Comment:
The JavaDoc for this method still says it returns a "map keyed by metric
name", but the method now returns a List<MetricPoint>. Please update the
`@return` text to reflect the new return type.
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/updater/metrics/storage/jdbc/JdbcMetricsRepository.java:
##########
@@ -230,208 +228,83 @@ private void executeSchemaSql(Connection connection,
String sqlContent) throws S
}
@Override
- public void storeTableMetrics(List<TableMetricWriteRequest> metrics) {
+ public void storeMetrics(List<MetricPoint> metrics) {
Preconditions.checkArgument(metrics != null, "metrics must not be null");
if (metrics.isEmpty()) {
return;
}
- String insertSql =
+ String tableInsertSql =
"INSERT INTO table_metrics (table_identifier, metric_name,
table_partition, metric_ts, metric_value) VALUES (?, ?, ?, ?, ?)";
+ String jobInsertSql =
+ "INSERT INTO job_metrics (job_identifier, metric_name, metric_ts,
metric_value) VALUES (?, ?, ?, ?)";
try (Connection conn = getConnection();
- PreparedStatement insertStmt = conn.prepareStatement(insertSql)) {
- int batchCount = 0;
- for (TableMetricWriteRequest request : metrics) {
- Preconditions.checkArgument(request != null, "table metric request
must not be null");
- validateWriteArguments(
- request.nameIdentifier(), request.metricName(),
request.partition(), request.metric());
- String normalizedIdentifier =
normalizeIdentifier(request.nameIdentifier());
- String normalizedMetricName =
normalizeMetricName(request.metricName());
- Optional<String> normalizedPartition =
normalizePartition(request.partition());
- MetricRecord metric = request.metric();
- insertStmt.setString(1, normalizedIdentifier);
- insertStmt.setString(2, normalizedMetricName);
- insertStmt.setString(3, normalizedPartition.orElse(null));
- insertStmt.setLong(4, metric.getTimestamp());
- insertStmt.setString(5, metric.getValue());
- insertStmt.addBatch();
- batchCount++;
- if (batchCount >= BATCH_UPDATE_SIZE) {
- insertStmt.executeBatch();
- batchCount = 0;
+ PreparedStatement tableInsertStmt =
conn.prepareStatement(tableInsertSql);
+ PreparedStatement jobInsertStmt = conn.prepareStatement(jobInsertSql))
{
+ int tableBatchCount = 0;
Review Comment:
storeMetrics() batches inserts into both table_metrics and job_metrics using
the same Connection, but it does not wrap the two statement batches in a
transaction. If a failure happens after one batch is executed, the call can
partially persist data across the two tables. Consider disabling auto-commit,
committing only after both batches succeed, and rolling back on exception to
keep this call atomic.
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/api/monitor/MetricsProvider.java:
##########
@@ -38,8 +37,7 @@ public interface MetricsProvider extends Provider {
* @param endTime end timestamp (seconds)
* @return map keyed by metric name, each containing a metric sample series
*/
- Map<String, List<MetricSample>> jobMetrics(
- NameIdentifier jobIdentifier, long startTime, long endTime);
+ List<MetricPoint> jobMetrics(NameIdentifier jobIdentifier, long startTime,
long endTime);
Review Comment:
The JavaDoc for this method still describes returning a "map keyed by metric
name", but the signature now returns a List<MetricPoint>. Please update the
`@return` description (and any related wording) to match the new return
type/semantics.
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/api/common/MetricPoint.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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.gravitino.maintenance.optimizer.api.common;
+
+import com.google.common.base.Preconditions;
+import java.util.Objects;
+import java.util.Optional;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.stats.StatisticValue;
+
+/** Immutable metric point for table, partition, or job metrics. */
+@DeveloperApi
+public final class MetricPoint {
+
+ /** Metric scope. */
+ public enum Scope {
+ TABLE,
+ PARTITION,
+ JOB
+ }
+
+ private final NameIdentifier identifier;
+ private final Scope scope;
+ private final Optional<PartitionPath> partitionPath;
+ private final String metricName;
+ private final StatisticValue<?> value;
Review Comment:
The PR description states the evaluator API is based on DataScope +
Map<String, List<MetricSample>> and that MetricSeries was removed, but the code
in this PR introduces/uses MetricSeries and MetricValueSample and still uses
MetricScope. Please reconcile the PR description with the actual API changes
(or adjust the code/rename types if the description is the intended design).
--
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]
