[#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics #1164

FANNG1 · 2023-12-14T13:40:54Z

What changes were proposed in this pull request?

introduce MetricsManager to manage storages
introduce IcebergMetricsFormatter to format metrics, support JSON for now.

Why are the changes needed?

Fix: #1187

Does this PR introduce any user-facing change?

no

How was this patch tested?

local env

jerryshao · 2023-12-15T06:32:43Z

What's the purpose of writing metrics to FS, not db?

FANNG1 · 2023-12-15T14:10:14Z

What's the purpose of writing metrics to FS, not db?

Analyzing the metrics with Spark or Python is easy if writing to FS.

FANNG1 · 2023-12-18T09:39:29Z

remove the specified storage implement, leaving the basic framework.

FANNG1 · 2023-12-19T02:21:16Z

@Clearvive @jerryshao , could you help to review this pr?

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

Clearvive · 2023-12-20T01:46:18Z

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

+
+  public static final ConfigEntry<Integer> ICEBERG_METRICS_STORAGE_RETAIN_DAYS =
+      new ConfigBuilder(IcebergMetricsManager.ICEBERG_METRICS_STORAGE_RETAIN_DAYS)
+          .doc("The retain days of Iceberg metrics")


Is it reasonable to keep the default setting based on the day dimension, and would it be more reasonable to configure it based on the hour dimension

keep metrics in days seems enough

qqqttt123 · 2023-12-20T02:55:40Z

Do we need a metrics storage for the Gravitino server?

...rc/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/IcebergObjectMapper.java

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

...g/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/IcebergRestUtils.java

FANNG1 · 2023-12-20T03:19:34Z

Do we need a metrics storage for the Gravitino server?

no, it's used to store Iceberg query or commit metrics not server metrics.

FANNG1 · 2023-12-20T03:54:47Z

@Clearvive @yuqi1129 , are comments addressed, please review again. thx.

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

Clearvive · 2023-12-20T04:10:50Z

LGTM except the few comments

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

jerryshao · 2023-12-21T02:49:03Z

...rc/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/IcebergObjectMapper.java

+    mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+    mapper.setPropertyNamingStrategy(new PropertyNamingStrategies.KebabCaseStrategy());
+    RESTSerializers.registerAll(mapper);
+    return mapper;


What is the purpose of change here, why do you need a new ObjectMapper?

The previouse implement(JsonUtils.objectMapper()) get a global ObjectMapper and applies Iceberg REST properties to it, this's not a good design. it may effect other ObjectMappers.

jerryshao · 2023-12-21T02:59:17Z

...g/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/IcebergRestUtils.java

+    LocalDateTime currentDateTime =
+        LocalDateTime.ofInstant(currentTimestamp, ZoneId.systemDefault());
+    LocalDateTime nextHourStartTime = currentDateTime.plusHours(1).withMinute(0).withSecond(0);
+    return nextHourStartTime.atZone(ZoneId.systemDefault()).toInstant();


What is the purpose of using LocalDateTime, it is quite simple to calculate the time hour's timestamp by using mod, like (curr_time + 3600000) - (curr_time + 3600000) % 3600000.

LocalDateTime seems more maintainable.

...java/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/EmptyMetricsStorage.java

jerryshao · 2023-12-21T03:02:20Z

.../com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsFormatter.java

+    this.icebergObjectMapper = IcebergObjectMapper.getInstance();
+  }
+
+  public String toJsonOrToString(MetricsReport metricsReport) {


The method name is weird.

How about toPrintableString()?

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

jerryshao · 2023-12-21T05:07:49Z

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

+  }
+
+  private void logMetrics(String message, MetricsReport metricsReport) {
+    LOG.info("{} {}", message, icebergMetricsFormatter.toJsonOrToString(metricsReport));


Looks like this IcebergMetricsFormatter is only used here, Is it necessary to have a class to define this?

IcebergMetricsFormatter is designed to provide variable format for other storages like KVStorage or SQLStorage

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

FANNG1 · 2023-12-21T09:40:46Z

@jerryshao , all comments are addressed, please help to review again.

FANNG1 · 2023-12-25T14:01:04Z

planing to do:

remove async mode configuration
add queue capacity configutation
use service loader to load specified metrics storage.

FANNG1 · 2023-12-26T03:44:12Z

planing to do:

remove async mode configuration

add queue capacity configutation

use service loader to load specified metrics storage.

supported in the lastest pr , cc @jerryshao

jerryshao · 2023-12-26T08:04:25Z

I think there's no need to use service loader to make it pluggable, using reflection should be enough.

FANNG1 · 2023-12-26T08:58:37Z

I think there's no need to use service loader to make it pluggable, using reflection should be enough.

replace service loader with reflection.

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

jerryshao · 2023-12-26T09:07:29Z

.../com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsFormatter.java

+  public String toJson(MetricsReport metricsReport) throws JsonProcessingException {
+    return icebergObjectMapper.writeValueAsString(metricsReport);
+  }
+}


If this class is only used for print logs, I don't think it is necessary to have a class, some helper methods should be enough.

I will use this class to serialize metrics report to json string in KVMetricsStorage in next pr. should I keep it as a class?

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsStorage.java

...ava/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/MemoryMetricsStorage.java

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/BlackHoleMetricsStore.java

jerryshao · 2023-12-27T06:15:24Z

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

+    }
+  }
+
+  public void save(MetricsReport metricsReport) {


This save method is quite confused with the method above, you'd better choose a better name.

Also you should put all the public method together, then all all private method.

jerryshao · 2023-12-27T06:21:56Z

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java

+        metricsWriterThread.join();
+      } catch (InterruptedException e) {
+        LOG.warn("Iceberg metrics manager is interrupted while join metrics writer thread.");
+        return;


You should not return directly here, you still need to execute the follow-up logic.

If IcebergMetricsManager is interrupted, I think it's better to quit as quickly as possible in case the other cleanup actions block the close.

At least you should close the iceberg metrics store, right?

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/BlackHoleMetricsStore.java

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java

FANNG1 marked this pull request as draft December 14, 2023 13:41

FANNG1 force-pushed the store-metric branch from f518125 to 1adec9b Compare December 18, 2023 02:02

FANNG1 changed the title ~~[#453] feat(lakehouse-iceberg): store Iceberg metrics in file storage~~ [#453] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics Dec 18, 2023

FANNG1 changed the title ~~[#453] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics~~ [#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics Dec 18, 2023

FANNG1 force-pushed the store-metric branch from 39ecf69 to 793d5e9 Compare December 18, 2023 09:36

FANNG1 requested review from Clearvive and jerryshao December 18, 2023 10:45

FANNG1 marked this pull request as ready for review December 18, 2023 12:03

Clearvive reviewed Dec 20, 2023

View reviewed changes

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java Outdated Show resolved Hide resolved

Clearvive reviewed Dec 20, 2023

View reviewed changes

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java Show resolved Hide resolved

Clearvive reviewed Dec 20, 2023

View reviewed changes

yuqi1129 reviewed Dec 20, 2023

View reviewed changes

Clearvive reviewed Dec 20, 2023

View reviewed changes

...va/com/datastrato/gravitino/catalog/lakehouse/iceberg/web/metrics/IcebergMetricsManager.java Outdated Show resolved Hide resolved

yuqi1129 reviewed Dec 20, 2023

View reviewed changes

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java Show resolved Hide resolved

jerryshao reviewed Dec 21, 2023

View reviewed changes

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java Outdated Show resolved Hide resolved

jerryshao reviewed Dec 21, 2023

View reviewed changes

FANNG1 force-pushed the store-metric branch from 62a6f8f to 9c42906 Compare December 21, 2023 08:07

qqqttt123 reviewed Dec 26, 2023

View reviewed changes

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java Show resolved Hide resolved

jerryshao reviewed Dec 26, 2023

View reviewed changes

FANNG1 force-pushed the store-metric branch from c9e4085 to 2326873 Compare December 26, 2023 12:22

jerryshao reviewed Dec 27, 2023

View reviewed changes

FANNG1 added 3 commits December 27, 2023 17:20

iceberg metrics

f04aa3a

fix comment

2467291

re position the method and rename dummy

2e3fcdd

FANNG1 force-pushed the store-metric branch from d0c95ac to 2e3fcdd Compare December 27, 2023 09:21

dummy

e97969d

jerryshao reviewed Dec 27, 2023

View reviewed changes

...-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergConfig.java Outdated Show resolved Hide resolved

FANNG1 added 2 commits December 27, 2023 21:59

close metrics store

89eb02a

chagne default value

254344b

jerryshao approved these changes Dec 28, 2023

View reviewed changes

jerryshao merged commit eff38b8 into apache:main Dec 28, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics #1164

[#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics #1164

FANNG1 commented Dec 14, 2023 •

edited

Loading

jerryshao commented Dec 15, 2023

FANNG1 commented Dec 15, 2023

FANNG1 commented Dec 18, 2023

FANNG1 commented Dec 19, 2023

Clearvive Dec 20, 2023

FANNG1 Dec 20, 2023

qqqttt123 commented Dec 20, 2023

FANNG1 commented Dec 20, 2023

FANNG1 commented Dec 20, 2023

Clearvive commented Dec 20, 2023

jerryshao Dec 21, 2023

FANNG1 Dec 21, 2023

jerryshao Dec 21, 2023 •

edited

Loading

FANNG1 Dec 21, 2023

jerryshao Dec 21, 2023

FANNG1 Dec 21, 2023

jerryshao Dec 21, 2023

FANNG1 Dec 21, 2023

FANNG1 commented Dec 21, 2023

FANNG1 commented Dec 25, 2023

FANNG1 commented Dec 26, 2023

jerryshao commented Dec 26, 2023

FANNG1 commented Dec 26, 2023

jerryshao Dec 26, 2023

FANNG1 Dec 26, 2023

jerryshao Dec 27, 2023

jerryshao Dec 27, 2023

FANNG1 Dec 27, 2023

jerryshao Dec 27, 2023

FANNG1 Dec 27, 2023

[#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics #1164

[#1187] feat(lakehouse-iceberg): the basic framework of storing Iceberg metrics #1164

Conversation

FANNG1 commented Dec 14, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerryshao commented Dec 15, 2023

FANNG1 commented Dec 15, 2023

FANNG1 commented Dec 18, 2023

FANNG1 commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Dec 20, 2023

FANNG1 commented Dec 20, 2023

FANNG1 commented Dec 20, 2023

Clearvive commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Dec 21, 2023

FANNG1 commented Dec 25, 2023

FANNG1 commented Dec 26, 2023

jerryshao commented Dec 26, 2023

FANNG1 commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Dec 14, 2023 •

edited

Loading

jerryshao Dec 21, 2023 •

edited

Loading