[4958] feat(iceberg): support event listener for Iceberg REST server #5002

FANNG1 · 2024-09-24T07:36:54Z

What changes were proposed in this pull request?

Integrate Event listener system to Iceberg REST server
only dispatch create table event like CreateIcebergTableEvent CreateIcebergTablePreEvent CreateIcebergTableFailureEvent

Why are the changes needed?

Fix: #4958

Does this PR introduce any user-facing change?

no

How was this patch tested?

add configuration to add IcebergEventLogger in event listener

gravitino.eventListener.names = iceberg
gravitino.eventListener.iceberg.class = org.apache.gravitino.iceberg.extension.IcebergEventLogger

run query will see event logs.

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

FANNG1 · 2024-10-16T03:56:42Z

@jerryshao please help to review when you have time, thanks

jerryshao · 2024-10-17T01:54:44Z

@mchades @yuqi1129 please help to review this PR first, thanks.

jerryshao · 2024-10-18T03:15:52Z

core/src/main/java/org/apache/gravitino/GravitinoEnv.java

+   * @param config The configuration object to initialize the environment.
+   */
+  public void initializeAllComponents(Config config) {
+    LOG.info("Initializing Gravitino all Environment...");


"full environement.."

jerryshao · 2024-10-18T03:20:28Z

...erg-rest-server/src/main/java/org/apache/gravitino/iceberg/extension/IcebergEventLogger.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class IcebergEventLogger implements EventListenerPlugin {


I think we don't have to had a IcebergEventLogger here for demo purpose. A better way is to add audit log support for iceberg rest server when Audit PR is merged.

jerryshao · 2024-10-18T03:28:27Z

...iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRestUtils.java

+
+  // remove the last '/' from the prefix, for example transform 'iceberg_catalog/' to
+  // 'iceberg_catalog'
+  private static String shelling(String rawPrefix) {


What's the meaning of shelling, it is hard to understand, can we use a better name?

jerryshao · 2024-10-18T03:30:32Z

...c/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableEventDispatcher.java

+    NameIdentifier nameIdentifier =
+        IcebergRestUtils.getGravitinoNameIdentifier(catalogName, tableIdentifier);


Do we support multiple level namespace for Iceberg rest server?

Do we need to add metalake to the name identifier?

1, Our implementation doesn't limit on the namespace levels.
2, Iceberg rest server doesn't introduce the concept of metalake for now, should we add a default metalake name?

use gravitino as default metalake name

jerryshao · 2024-10-18T03:31:55Z

...in/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationDispatcher.java

+   * @return A {@link LoadTableResponse} object containing the result of the operation.
+   */
+  LoadTableResponse createTable(
+      String catalogName, Namespace namespace, CreateTableRequest createTableRequest);


So you only support one operation for now, right?

Will you do this in the follow-up PR?

yes, will support the other table operations in another PR

jerryshao · 2024-10-18T03:34:02Z

...ain/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationProcessor.java

+import org.apache.iceberg.rest.responses.LoadTableResponse;
+
+/**
+ * The {@code IcebergTableOperationProcessor} locates the corresponding {@code


What is the meaning of "locate"?

jerryshao · 2024-10-18T03:35:02Z

...ain/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationProcessor.java

+ * The {@code IcebergTableOperationProcessor} locates the corresponding {@code
+ * IcebergCatalogWrapper} based on the prefix in order to perform the actual table operations.
+ */
+public class IcebergTableOperationProcessor implements IcebergTableOperationDispatcher {


Is it better to use Executor than Processor?

jerryshao · 2024-10-18T03:39:23Z

...server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateTablePreEvent.java

+  public IcebergCreateTablePreEvent(
+      String user, NameIdentifier resourceIdentifier, CreateTableRequest createTableRequest) {
+    super(user, resourceIdentifier);
+    this.createTableRequest = createTableRequest;


Do you need to clone this object here?

No, it's designed to allow the user to change the object.

jerryshao · 2024-10-18T03:42:19Z

...t-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergTableFailureEvent.java

+
+/** Represent a failure event when do Iceberg table operation failed. */
+@DeveloperApi
+public class IcebergTableFailureEvent extends IcebergRESTFailureEvent {


Why do we need one IcebergRESTFailureEvent and another IcebergTableFailureEvent ?

Another thing is that the class name is too verbose, I think it can be simplified like IcebergTableFailureEvent, IcebergTablePostEvent, IcebergFailureEvent...

to form a event hierarchy, FailureEvent -> IcebergRESTFailureEvent -> IcebergTableFailureEvent -> IcebergCreateTableFailureEvent

jerryshao · 2024-10-18T03:42:42Z

...rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergTablePostEvent.java

+import org.apache.gravitino.NameIdentifier;
+
+/** Represents an abstract table post event in Gravitino Iceberg REST server. */
+public abstract class IcebergTablePostEvent extends IcebergRESTPostEvent {


jerryshao · 2024-10-18T10:11:53Z

core/src/main/java/org/apache/gravitino/GravitinoEnv.java

+   * @param config The configuration object to initialize the environment.
+   */
+  public void initializeFullComponents(Config config) {
+    LOG.info("Initializing Gravitino full Environment...");


"environment..."

jerryshao · 2024-10-18T11:21:54Z

...iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRestUtils.java

+    String prefix = normalizePrefix(rawPrefix);
+    Preconditions.checkArgument(
+        !IcebergConstants.ICEBERG_REST_DEFAULT_CATALOG.equals(prefix),
+        String.format("%s is conflict with reserved key, please replace it", prefix));


"...is conflicted with reserved catalog name"

jerryshao · 2024-10-18T11:27:31Z

...st-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRESTFailureEvent.java

+
+/** Represents an abstract failure event in Gravitino Iceberg REST server. */
+@DeveloperApi
+public abstract class IcebergRESTFailureEvent extends FailureEvent {


Maybe IcebergFailureEvent is enough.

jerryshao · 2024-10-18T11:29:34Z

This seems has a lot of conflicts, please fix it first.

FANNG1 · 2024-10-20T09:34:32Z

This seems has a lot of conflicts, please fix it first.

updated the PR, please help to review again

jerryshao · 2024-10-21T09:59:30Z

iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java

@@ -205,7 +205,7 @@ public class IcebergConfig extends Config implements OverwriteDefaultConfig {
  public static final ConfigEntry<String> ICEBERG_REST_CATALOG_CONFIG_PROVIDER =
      new ConfigBuilder(IcebergConstants.ICEBERG_REST_CATALOG_CONFIG_PROVIDER)
          .doc(
-              "Catalog provider class name, you can develop a class that implements `IcebergCatalogConfigProvider` and add the corresponding jar file to the Iceberg REST service classpath directory.")
+              "Catalog provider class name, you can develop a class that implements `IcebergConfigProvider` and add the corresponding jar file to the Iceberg REST service classpath directory.")


This is too long, you need to make it less than 100 chars.

jerryshao · 2024-10-21T10:04:43Z

iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java

+    Map<String, String> configProperties = icebergConfig.getAllConfig();
+    EventBus eventBus = GravitinoEnv.getInstance().eventBus();
+    this.configProvider = IcebergConfigProviderFactory.create(configProperties);
+    configProvider.initialize(configProperties);
+    String metalakeName = configProvider.getMetalakeName();
+    this.icebergCatalogWrapperManager =
+        new IcebergCatalogWrapperManager(configProperties, configProvider);
+    this.icebergMetricsManager = new IcebergMetricsManager(icebergConfig);
+    IcebergTableOperationExecutor icebergTableOperationExecutor =
+        new IcebergTableOperationExecutor(icebergCatalogWrapperManager);
+    IcebergTableEventDispatcher icebergTableEventDispatcher =
+        new IcebergTableEventDispatcher(icebergTableOperationExecutor, eventBus, metalakeName);


Can we add some necessary blank line in this code block to make this code block easy to read.

jerryshao · 2024-10-21T10:52:43Z

...rc/main/java/org/apache/gravitino/iceberg/service/provider/IcebergConfigProviderFactory.java

+
+public class IcebergConfigProviderFactory {
+  public static final Logger LOG = LoggerFactory.getLogger(IcebergConfigProviderFactory.class);
+  private static final ImmutableMap<String, String> ICEBERG_CATALOG_CONFIG_PROVIDER_NAMES =


Add a blank line above.

jerryshao · 2024-10-21T10:53:43Z

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

    LOG.info(
-        "Create Iceberg table, namespace: {}, create table request: {}, accessDelegation: {}, isCredentialVending: {}",
-        namespace,
+        "Create Iceberg table, catalog: {}, namespace: {}, create table request: {}, accessDelegation: {}, isCredentialVending: {}",


I guess this line is also too long.

…pache#5002) ### What changes were proposed in this pull request? 1. Integrate Event listener system to Iceberg REST server 2. only dispatch create table event like `CreateIcebergTableEvent` `CreateIcebergTablePreEvent` `CreateIcebergTableFailureEvent` ### Why are the changes needed? Fix: apache#4958 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? 1. add configuration to add `IcebergEventLogger` in event listener ``` gravitino.eventListener.names = iceberg gravitino.eventListener.iceberg.class = org.apache.gravitino.iceberg.extension.IcebergEventLogger ``` 2. run query will see event logs. --------- Co-authored-by: Qi Yu <[email protected]>

FANNG1 marked this pull request as draft September 24, 2024 07:38

FANNG1 changed the title ~~[SIP] support event listener for Iceberg REST server~~ [SIP] feat(iceberg): support event listener for Iceberg REST server Sep 24, 2024

FANNG1 commented Oct 1, 2024

View reviewed changes

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java Outdated Show resolved Hide resolved

FANNG1 force-pushed the iceberg-event branch from f4a9fdb to 4fd7a32 Compare October 16, 2024 03:41

FANNG1 changed the title ~~[SIP] feat(iceberg): support event listener for Iceberg REST server~~ [4958] feat(iceberg): support event listener for Iceberg REST server Oct 16, 2024

FANNG1 marked this pull request as ready for review October 16, 2024 03:55

FANNG1 force-pushed the iceberg-event branch from ac00f09 to 66b234a Compare October 16, 2024 06:48

mchades assigned FANNG1 Oct 16, 2024

mchades requested review from yuqi1129 and mchades October 17, 2024 03:45

jerryshao reviewed Oct 18, 2024

View reviewed changes

FANNG1 added 2 commits October 18, 2024 22:55

support iceberg pre event listener

f5beda9

fix comment

9a5a18f

FANNG1 force-pushed the iceberg-event branch from 8f5234c to 9a5a18f Compare October 20, 2024 08:25

FANNG1 added 3 commits October 20, 2024 16:36

fix comment

f5690e3

fix comment

fe508a6

fix comment

641f9e1

FANNG1 added 3 commits October 20, 2024 17:39

fix comment

fedb8dd

fix comment

c11ad89

fix comment

12baeeb

jerryshao reviewed Oct 21, 2024

View reviewed changes

fix comment

9a3e988

jerryshao approved these changes Oct 21, 2024

View reviewed changes

Merge branch 'main' into iceberg-event

6a6eade

jerryshao merged commit d35ab31 into apache:main Oct 21, 2024
26 checks passed

FANNG1 mentioned this pull request Oct 22, 2024

[Subtask] support pre-hook for IcebergRESTServer #5050

Closed

		NameIdentifier nameIdentifier =
		IcebergRestUtils.getGravitinoNameIdentifier(catalogName, tableIdentifier);

[4958] feat(iceberg): support event listener for Iceberg REST server #5002

[4958] feat(iceberg): support event listener for Iceberg REST server #5002

Conversation

FANNG1 commented Sep 24, 2024 • 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?

FANNG1 commented Oct 16, 2024

jerryshao commented Oct 17, 2024

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Oct 18, 2024

FANNG1 commented Oct 20, 2024

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

Choose a reason for hiding this comment

FANNG1 commented Sep 24, 2024 •

edited

Loading