[#2730] Split Iceberg REST service #4005

FANNG1 · 2024-06-28T07:49:10Z

What changes were proposed in this pull request?

Split IcebergRESTService into one separate module, it could be managed with or without Gravitino server.

For user

provide an isolated Iceberg REST server package or Gravitino package with directory iceberg-rest-server which contains Iceberg REST server. They both contain isolated config files and start&stop script.
The configuration items start with gravitino.iceberg-rest. are treated as Iceberg REST server configs. like gravitino.iceberg-rest.catalog-backend, gravitino.iceberg-rest.httpPort, etc. Configurations for Iceberg REST server are same in isolated package and Gravitino package.

For developer

The core is RESTService which start JettyServer for Iceberg REST server,

For isolated Iceberg REST server module, the entrypoint is IcebergRESTServer which provides configuration, metricsSystem for RESTService.
For GravitinoServer, RESTService is managed as a AuxiliaryService.

Why are the changes needed?

Iceberg rest service is managed as an auxiliary service in Gravitino server , for the users who want to use Iceberg REST service only, it introduces an extra burden.

Fix: #2730

Does this PR introduce any user-facing change?

yes, will add document in #4113

How was this patch tested?

existing tests
test in local env as a Iceberg REST server

related docs

https://docs.google.com/document/d/1lyJwMaaJKfMqtnH9c7LwvnOHRKm7gh8Al4Sw3T1DFjM/edit

lmccay · 2024-07-03T19:54:07Z

@FANNG1 - I am interested in this change and would like to read some background on it. Is there a [email protected] thread to DISCUSS such a large change? A design doc or proposal in wiki? Can you update the PR template with some details of what this is doing and why?

FANNG1 · 2024-07-03T23:46:55Z

@FANNG1 - I am interested in this change and would like to read some background on it. Is there a [email protected] thread to DISCUSS such a large change? A design doc or proposal in wiki? Can you update the PR template with some details of what this is doing and why?

Hi, @lmccay , I have updated the doc in the PR template, you could refer to it.

FANNG1 · 2024-07-05T06:35:28Z

bin/gravitino.sh


 JAVA_OPTS+=" -Dfile.encoding=UTF-8"
 JAVA_OPTS+=" -Dlog4j2.configurationFile=file://${GRAVITINO_CONF_DIR}/log4j2.properties"
 JAVA_OPTS+=" -Dgravitino.log.path=${GRAVITINO_LOG_DIR} ${GRAVITINO_MEM}"
+JAVA_OPTS+=" -Dgravitino.server.name=${GRAVITINO_SIMPLE_SERVER_NAME}"


used to specify log file name

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

FANNG1 · 2024-07-19T08:25:26Z

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

bin/gravitino-iceberg-rest-server.sh

jerryshao · 2024-07-19T11:00:12Z

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

+    eventListenerManager.init(
+        config.getConfigsWithPrefix(EventListenerManager.GRAVITINO_EVENT_LISTENER_PREFIX));
+    this.eventBus = eventListenerManager.createEventBus();
+  }


I would think that access control manager is also needed for the Iceberg rest server. But it requires many refactorings, we can do it later.

yes, this is the next phase for Iceberg REST service.

jerryshao · 2024-07-22T12:15:59Z

catalogs/catalog-lakehouse-iceberg/build.gradle.kts

  implementation(libs.bundles.jersey)
+  implementation(libs.bundles.jetty)
+


Please remove this blank line.

jerryshao · 2024-07-22T12:26:26Z

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

-      this.accessControlManager = new AccessControlManager(entityStore, idGenerator, config);
-    } else {
-      this.accessControlManager = null;
+    initBaseComponent();


"initBaseComponents" and "initGravitinoServerComponents"

jerryshao · 2024-07-22T12:34:21Z

core/src/main/java/org/apache/gravitino/auxiliary/AuxiliaryServiceManager.java

+            name ->
+                config.getAllConfig().forEach((k, v) -> extractConfig(serviceConfigs, name, k, v)));
+    return serviceConfigs;
+  }


I think you should:

Update the server config doc to describe the new config names and declare that old keys are still workable.

Here in the code, when old configs are configured, output a warn log to users.

will use separate PR to add document

done

jerryshao · 2024-07-22T12:37:01Z

core/src/test/java/org/apache/gravitino/auxiliary/TestAuxiliaryServiceManager.java

+            "test4",
+            "b.test2",
+            "test2"),
+        serviceConfigs);


If old and new configs are both set, who will take precedence, who will overwrite who? I think you need to clarify this behavior and add UT to cover this scenario.

It will throw exception and add UT for it

I think you should honor overwrite mechanism, not throw an exception, I think throwing an exception is too severe, just adding warning logs and using new to overwrite old should be enough, what do you think?

ok, updated

FANNG1 · 2024-07-23T09:19:16Z

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

FANNG1 force-pushed the split branch 10 times, most recently from 843e9f4 to 8402bec Compare July 2, 2024 10:17

FANNG1 closed this Jul 3, 2024

FANNG1 reopened this Jul 3, 2024

FANNG1 force-pushed the split branch 6 times, most recently from 8a3624b to 91f9938 Compare July 3, 2024 08:02

FANNG1 force-pushed the split branch 4 times, most recently from 6a92403 to 4b951b9 Compare July 4, 2024 08:45

FANNG1 mentioned this pull request Jul 4, 2024

[Improvement] IcebergCatalogBackend class not found in sparkIT embedded mode if moving IcebergCatalogBackend to iceberg-common module #4084

Closed

FANNG1 force-pushed the split branch from 3b8ffe6 to 98dc1e4 Compare July 5, 2024 03:55

FANNG1 commented Jul 5, 2024

View reviewed changes

FANNG1 changed the title ~~[SIP] Split Iceberg REST service~~ [#2730] Split Iceberg REST service Jul 5, 2024

FANNG1 marked this pull request as draft July 5, 2024 08:46

FANNG1 force-pushed the split branch from 9e8b10d to 7c49c9e Compare July 5, 2024 11:18

jerryshao reviewed Jul 18, 2024

View reviewed changes

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

FANNG1 force-pushed the split branch 3 times, most recently from c079c5e to f4e6969 Compare July 19, 2024 08:14

FANNG1 force-pushed the split branch 2 times, most recently from 4a0a796 to 10de2c9 Compare July 19, 2024 10:34

jerryshao reviewed Jul 19, 2024

View reviewed changes

FANNG1 force-pushed the split branch from 10de2c9 to 45a9610 Compare July 19, 2024 11:41

jerryshao reviewed Jul 22, 2024

View reviewed changes

FANNG1 added 11 commits July 23, 2024 15:06

support split iceberg rest service

9aaaeb3

fix comment

5a19394

fix comments

47e6b89

xx

aa0c655

xx

6decc4f

polish

b59a9bb

polish

10c9012

fix comment

859f849

overwrite not throw exception

3cc76da

polish log

23e5316

use catalog-common

30c0bda

FANNG1 force-pushed the split branch from e814efb to 30c0bda Compare July 23, 2024 07:11

jerryshao approved these changes Jul 23, 2024

View reviewed changes

jerryshao merged commit 2353a87 into apache:main Jul 23, 2024
33 checks passed

jerryshao assigned FANNG1 Jul 23, 2024

FANNG1 added the ICEBERG-REST-SERVER label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2730] Split Iceberg REST service #4005

[#2730] Split Iceberg REST service #4005

FANNG1 commented Jun 28, 2024 •

edited

Loading

lmccay commented Jul 3, 2024

FANNG1 commented Jul 3, 2024

FANNG1 Jul 5, 2024

FANNG1 commented Jul 19, 2024

jerryshao Jul 19, 2024

FANNG1 Jul 19, 2024 •

edited

Loading

jerryshao Jul 22, 2024

FANNG1 Jul 22, 2024

jerryshao Jul 22, 2024

FANNG1 Jul 22, 2024

jerryshao Jul 22, 2024

FANNG1 Jul 22, 2024

jerryshao Jul 22, 2024

FANNG1 Jul 22, 2024

jerryshao Jul 23, 2024 •

edited

Loading

FANNG1 Jul 23, 2024

FANNG1 commented Jul 23, 2024

		implementation(libs.bundles.jersey)
		implementation(libs.bundles.jetty)

[#2730] Split Iceberg REST service #4005

[#2730] Split Iceberg REST service #4005

Conversation

FANNG1 commented Jun 28, 2024 • edited Loading

What changes were proposed in this pull request?

For user

For developer

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

related docs

lmccay commented Jul 3, 2024

FANNG1 commented Jul 3, 2024

Choose a reason for hiding this comment

FANNG1 commented Jul 19, 2024

Choose a reason for hiding this comment

FANNG1 Jul 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Jul 23, 2024

FANNG1 commented Jun 28, 2024 •

edited

Loading

FANNG1 Jul 19, 2024 •

edited

Loading

jerryshao Jul 23, 2024 •

edited

Loading