Copilot commented on code in PR #10138:
URL: https://github.com/apache/gravitino/pull/10138#discussion_r2877876876
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/monitor/callback/ConsoleMonitorCallback.java:
##########
@@ -67,26 +67,24 @@ public void close() throws Exception {}
private String formatMetrics(Map<String, List<MetricSample>> metrics) {
if (metrics == null || metrics.isEmpty()) {
- return "{}";
+ return "[]";
Review Comment:
formatMetrics(...) returns "[]" for an empty metrics map, but returns a
map-like string ("{k=[...]}" ) when non-empty. This makes the output
inconsistent and misleading (empty map should format as "{}" or match the
non-empty representation).
```suggestion
return "{}";
```
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/monitor/evaluator/GravitinoMetricsEvaluator.java:
##########
@@ -294,31 +292,30 @@ private static String formatRules(Map<RuleScope,
Map<String, RuleConfig>> rulesB
.collect(Collectors.joining(", "));
}
- private static List<MetricSample> sanitizeSamples(List<MetricSample>
samples) {
- return samples == null ? List.of() : samples;
- }
-
- private static List<MetricSample> findMetricSamples(
- Map<String, List<MetricSample>> metrics, String normalizedMetricName) {
- if (metrics == null || metrics.isEmpty()) {
- return List.of();
- }
- List<MetricSample> exactMatch = metrics.get(normalizedMetricName);
- if (exactMatch != null) {
- return exactMatch;
+ private static void validateInputs(
+ DataScope scope,
+ Map<String, List<MetricSample>> beforeMetrics,
+ Map<String, List<MetricSample>> afterMetrics) {
+ if (scope == null) {
+ throw new IllegalArgumentException("scope must not be null");
}
- for (Map.Entry<String, List<MetricSample>> entry : metrics.entrySet()) {
- if (normalizeMetricName(entry.getKey()).equals(normalizedMetricName)) {
- return entry.getValue();
- }
+ if (beforeMetrics == null || afterMetrics == null) {
+ throw new IllegalArgumentException("beforeMetrics and afterMetrics must
not be null");
}
- return List.of();
}
private static String normalizeMetricName(String metricName) {
return metricName == null ? "" :
metricName.trim().toLowerCase(Locale.ROOT);
}
+ private static List<MetricSample> samples(
+ Map<String, List<MetricSample>> metrics, String metricName) {
+ if (metrics.isEmpty()) {
+ return List.of();
+ }
+ return metrics.getOrDefault(normalizeMetricName(metricName), List.of());
Review Comment:
samples(...) now does a direct lookup using the normalized rule metric name
(metrics.getOrDefault(normalizeMetricName(...))). This makes evaluation
dependent on callers already normalizing the Map keys; mixed-case metric keys
will no longer match and the rule will be skipped. Consider normalizing keys in
one place (e.g., build a normalized view/copy of the input map) or falling back
to a case-insensitive scan when the direct lookup misses.
```suggestion
if (metrics == null || metrics.isEmpty()) {
return List.of();
}
String normalizedMetricName = normalizeMetricName(metricName);
// Fast path: try exact key match with the provided metricName
if (metricName != null) {
List<MetricSample> direct = metrics.get(metricName);
if (direct != null) {
return direct;
}
}
// Second fast path: try direct lookup using the normalized name
List<MetricSample> normalizedDirect = metrics.get(normalizedMetricName);
if (normalizedDirect != null) {
return normalizedDirect;
}
// Fallback: scan entries comparing normalized keys to handle mixed-case
map keys
for (Map.Entry<String, List<MetricSample>> entry : metrics.entrySet()) {
if (normalizeMetricName(entry.getKey()).equals(normalizedMetricName)) {
return entry.getValue();
}
}
return List.of();
```
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/updater/metrics/TestGravitinoMetricsUpdater.java:
##########
@@ -191,4 +214,14 @@ private MetricsRepository
getMetricsRepository(GravitinoMetricsUpdater updater)
field.setAccessible(true);
return (MetricsRepository) field.get(updater);
}
+
+ private PartitionPath parsePartitionPath(String partition) {
+ String[] entries = partition.split("/");
+ List<PartitionEntry> partitionEntries = new
java.util.ArrayList<>(entries.length);
+ for (String entry : entries) {
+ String[] kv = entry.split("=", 2);
+ partitionEntries.add(new PartitionEntryImpl(kv[0], kv[1]));
Review Comment:
This helper uses a fully-qualified type (new java.util.ArrayList<>(...))
inside the method. Prefer adding the standard import (java.util.ArrayList) and
using ArrayList directly to match the project's Java import guideline and keep
the code consistent/readable.
--
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]
