Copilot commented on code in PR #10138:
URL: https://github.com/apache/gravitino/pull/10138#discussion_r2875981160
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/updater/metrics/storage/BaseGenericJdbcMetricsRepositoryTest.java:
##########
@@ -308,18 +311,98 @@ protected List<String> getMetricValues(List<MetricRecord>
metrics) {
return metrics.stream().map(MetricRecord::getValue).toList();
}
+ protected Map<String, List<MetricRecord>> getTableMetrics(
+ NameIdentifier nameIdentifier, long fromSecs, long toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(MetricScope.forTable(nameIdentifier), fromSecs,
toSecs));
+ }
+
+ protected Map<String, List<MetricRecord>> getPartitionMetrics(
+ NameIdentifier nameIdentifier, String partition, long fromSecs, long
toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(
+ MetricScope.forPartition(nameIdentifier,
parsePartitionPath(partition)),
+ fromSecs,
+ toSecs));
+ }
+
+ protected Map<String, List<MetricRecord>> getJobMetrics(
+ NameIdentifier nameIdentifier, long fromSecs, long toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(MetricScope.forJob(nameIdentifier), fromSecs,
toSecs));
+ }
+
protected void storeTableMetric(
NameIdentifier nameIdentifier,
String metricName,
Optional<String> partition,
MetricRecord metric) {
- storage.storeTableMetrics(
- List.of(new TableMetricWriteRequest(nameIdentifier, metricName,
partition, metric)));
+ MetricPoint metricPoint;
+ if (metric == null) {
+ metricPoint = null;
+ } else if (partition != null && partition.isPresent()) {
+ metricPoint =
+ MetricPoint.forPartition(
+ nameIdentifier,
+ parsePartitionPath(partition.get()),
+ metricName,
+ toStatisticValue(metric.getValue()),
+ metric.getTimestamp());
+ } else {
+ metricPoint =
+ MetricPoint.forTable(
+ nameIdentifier,
+ metricName,
+ toStatisticValue(metric.getValue()),
+ metric.getTimestamp());
+ }
+ storage.storeMetrics(List.of(metricPoint));
}
protected void storeJobMetric(
NameIdentifier nameIdentifier, String metricName, MetricRecord metric) {
- storage.storeJobMetrics(List.of(new JobMetricWriteRequest(nameIdentifier,
metricName, metric)));
+ MetricPoint metricPoint =
+ metric == null
+ ? null
+ : MetricPoint.forJob(
+ nameIdentifier,
+ metricName,
+ toStatisticValue(metric.getValue()),
+ metric.getTimestamp());
+ storage.storeMetrics(List.of(metricPoint));
+ }
Review Comment:
storeJobMetric can set metricPoint to null when metric is null, but then
passes List.of(metricPoint) into storage.storeMetrics(...). List.of(...) throws
NullPointerException on null elements, which will break the tests that expect
IllegalArgumentException for null metrics. Adjust the helper to avoid List.of
when metricPoint may be null (e.g., use a list type that permits null or handle
the null case separately).
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/updater/metrics/storage/BaseGenericJdbcMetricsRepositoryTest.java:
##########
@@ -308,18 +311,98 @@ protected List<String> getMetricValues(List<MetricRecord>
metrics) {
return metrics.stream().map(MetricRecord::getValue).toList();
}
+ protected Map<String, List<MetricRecord>> getTableMetrics(
+ NameIdentifier nameIdentifier, long fromSecs, long toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(MetricScope.forTable(nameIdentifier), fromSecs,
toSecs));
+ }
+
+ protected Map<String, List<MetricRecord>> getPartitionMetrics(
+ NameIdentifier nameIdentifier, String partition, long fromSecs, long
toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(
+ MetricScope.forPartition(nameIdentifier,
parsePartitionPath(partition)),
+ fromSecs,
+ toSecs));
+ }
+
+ protected Map<String, List<MetricRecord>> getJobMetrics(
+ NameIdentifier nameIdentifier, long fromSecs, long toSecs) {
+ return convertToMetricRecords(
+ storage.getMetrics(MetricScope.forJob(nameIdentifier), fromSecs,
toSecs));
+ }
+
protected void storeTableMetric(
NameIdentifier nameIdentifier,
String metricName,
Optional<String> partition,
MetricRecord metric) {
- storage.storeTableMetrics(
- List.of(new TableMetricWriteRequest(nameIdentifier, metricName,
partition, metric)));
+ MetricPoint metricPoint;
+ if (metric == null) {
+ metricPoint = null;
+ } else if (partition != null && partition.isPresent()) {
+ metricPoint =
+ MetricPoint.forPartition(
+ nameIdentifier,
+ parsePartitionPath(partition.get()),
+ metricName,
+ toStatisticValue(metric.getValue()),
+ metric.getTimestamp());
+ } else {
+ metricPoint =
+ MetricPoint.forTable(
+ nameIdentifier,
+ metricName,
+ toStatisticValue(metric.getValue()),
+ metric.getTimestamp());
+ }
+ storage.storeMetrics(List.of(metricPoint));
}
Review Comment:
storeTableMetric builds metricPoint=null when metric is null, but then calls
storage.storeMetrics(List.of(metricPoint)). List.of(...) rejects null elements
and will throw NullPointerException, so the validation tests that expect
IllegalArgumentException from the repository will fail. Use a list
implementation that allows null (so the repository can validate), or branch and
call storage.storeMetrics(...) with a nullable element via
Arrays.asList/Collections.singletonList, or directly assertThrows around the
storage call when metric is null.
--
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]
