[#411] test: support E2E tests for IcebergREST server #416

Clearvive · 2023-09-19T07:39:33Z

What changes were proposed in this pull request?

support E2E tests for IcebergREST server

github-actions · 2023-09-19T11:51:02Z

Code Coverage Report

Overall Project	62.25% `-0.6%`	🟢
Files changed	22.43%	🔴

Module	Coverage
catalog-lakehouse	79.48% `-13.45%`	🔴
core	71.48% `-0.06%`	🟢

Files

Module	File	Coverage
catalog-lakehouse	IcebergRESTConfig.java	100%	🟢
	IcebergTableOps.java	80.34% `-10.67%`	🔴
	IcebergCatalogUtil.java	53.33% `-42.67%`	🔴
	IcebergAuxiliaryService.java	0%	🔴
core	Config.java	38.13% `-2.33%`	🟢

xunliu · 2023-09-24T10:40:17Z

...house/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergRESTConfig.java

+
+public class IcebergRESTConfig extends Config {
+  public static final ConfigEntry<Integer> ICEBERG_REST_SERVER_HTTP_PORT =
+      new ConfigBuilder("serverPort")


Why the key ICEBERG_REST_SERVER_HTTP_PORT of the IcebergRESTConfig does not match the key of the Graviton, e.g. graviton.server.webserver.host, graviton.server.webserver.httpPort, etc?

And because ICEBERG_REST_SERVER_HTTP_PORT("serverPort") does not begin graviton.****** prefix, And let you need to change
public void Config::loadFromMap(Map<String, String> map) {
to
public void Config::loadFromMap(Map<String, String> map, boolean checkPrefix) {
??

ICEBERG_REST_SERVER_HTTP_PORT actually is 'graviton.auxService.GravitonIcebergREST.httpPort' in graviton.conf, the prefix is removed by AuxiliaryServiceManager

private void registerAuxServices(Map<String, String> config) { String auxServiceNames = config.getOrDefault(AUX_SERVICE_NAMES, ""); splitter .omitEmptyStrings() .trimResults() .splitToStream(auxServiceNames) .forEach( auxServiceName -> registerAuxService( auxServiceName, MapUtils.getPrefixMap(config, DOT.join(auxServiceName, "")))); }

xunliu · 2023-09-24T10:43:22Z

catalog-lakehouse/build.gradle.kts

+    implementation(libs.hadoop2.client) {
+        exclude("org.apache.avro", "avro")
+        exclude("org.slf4j", "slf4j-log4j12")
+    }


hi @yunqing-wei, libs.hadoop2.client have many dependencies, I guess maybe org.apache.hadoop:hadoop-mapreduce-client-app:2.7.3, org.apache.hadoop:hadoop-mapreduce-client-core:2.7.3 (*), org.apache.hadoop:hadoop-mapreduce-client-jobclient:2.7.3, org.apache.hadoop:hadoop-yarn-api:2.7.3 (*) we didn't use it.
@sandflee Please check it. whether some JAR need to be excluded?

cd catalog-lakehouse ../gradlew dependencies ...... \--- org.apache.hadoop:hadoop-client:2.7.3 +--- org.apache.hadoop:hadoop-common:2.7.3 (*) +--- org.apache.hadoop:hadoop-hdfs:2.7.3 | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | +--- org.mortbay.jetty:jetty-util:6.1.26 | +--- commons-cli:commons-cli:1.2 | +--- commons-codec:commons-codec:1.4 -> 1.9 | +--- commons-io:commons-io:2.4 | +--- commons-lang:commons-lang:2.6 | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | +--- log4j:log4j:1.2.17 | +--- com.google.protobuf:protobuf-java:2.5.0 | +--- org.codehaus.jackson:jackson-core-asl:1.9.13 | +--- org.codehaus.jackson:jackson-mapper-asl:1.9.13 (*) | +--- xmlenc:xmlenc:0.52 | +--- io.netty:netty:3.6.2.Final -> 3.7.0.Final | +--- io.netty:netty-all:4.0.23.Final | +--- xerces:xercesImpl:2.9.1 | | \--- xml-apis:xml-apis:1.3.04 | +--- org.apache.htrace:htrace-core:3.1.0-incubating | \--- org.fusesource.leveldbjni:leveldbjni-all:1.8 +--- org.apache.hadoop:hadoop-mapreduce-client-app:2.7.3 | +--- org.apache.hadoop:hadoop-mapreduce-client-common:2.7.3 | | +--- org.apache.hadoop:hadoop-yarn-common:2.7.3 | | | +--- org.apache.hadoop:hadoop-yarn-api:2.7.3 | | | | +--- commons-lang:commons-lang:2.6 | | | | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | | | | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | | | | \--- com.google.protobuf:protobuf-java:2.5.0 | | | +--- javax.xml.bind:jaxb-api:2.2.2 (*) | | | +--- org.apache.commons:commons-compress:1.4.1 -> 1.9 | | | +--- commons-lang:commons-lang:2.6 | | | +--- javax.servlet:servlet-api:2.5 | | | +--- commons-codec:commons-codec:1.4 -> 1.9 | | | +--- org.mortbay.jetty:jetty-util:6.1.26 | | | +--- com.sun.jersey:jersey-core:1.9 | | | +--- com.sun.jersey:jersey-client:1.9 | | | | \--- com.sun.jersey:jersey-core:1.9 | | | +--- org.codehaus.jackson:jackson-core-asl:1.9.13 | | | +--- org.codehaus.jackson:jackson-mapper-asl:1.9.13 (*) | | | +--- org.codehaus.jackson:jackson-jaxrs:1.9.13 (*) | | | +--- org.codehaus.jackson:jackson-xc:1.9.13 (*) | | | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | | | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | | | +--- commons-cli:commons-cli:1.2 | | | +--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 | | | +--- com.google.protobuf:protobuf-java:2.5.0 | | | +--- commons-io:commons-io:2.4 | | | +--- com.google.inject:guice:3.0 | | | | +--- javax.inject:javax.inject:1 | | | | +--- aopalliance:aopalliance:1.0 | | | | \--- org.sonatype.sisu.inject:cglib:2.2.1-v20090111 | | | | \--- asm:asm:3.1 | | | +--- com.sun.jersey:jersey-server:1.9 (*) | | | +--- com.sun.jersey:jersey-json:1.9 (*) | | | +--- com.sun.jersey.contribs:jersey-guice:1.9 | | | | +--- javax.inject:javax.inject:1 | | | | +--- com.google.inject:guice:3.0 (*) | | | | \--- com.sun.jersey:jersey-server:1.9 (*) | | | \--- log4j:log4j:1.2.17 | | +--- org.apache.hadoop:hadoop-yarn-client:2.7.3 | | | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | | | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | | | +--- commons-lang:commons-lang:2.6 | | | +--- commons-cli:commons-cli:1.2 | | | +--- log4j:log4j:1.2.17 | | | +--- org.apache.hadoop:hadoop-yarn-api:2.7.3 (*) | | | \--- org.apache.hadoop:hadoop-yarn-common:2.7.3 (*) | | +--- org.apache.hadoop:hadoop-mapreduce-client-core:2.7.3 | | | +--- org.apache.hadoop:hadoop-yarn-common:2.7.3 (*) | | | +--- com.google.protobuf:protobuf-java:2.5.0 | | | \--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 | | +--- org.apache.hadoop:hadoop-yarn-server-common:2.7.3 | | | +--- org.apache.hadoop:hadoop-yarn-api:2.7.3 (*) | | | +--- org.apache.hadoop:hadoop-yarn-common:2.7.3 (*) | | | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | | | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | | | +--- com.google.protobuf:protobuf-java:2.5.0 | | | +--- org.apache.zookeeper:zookeeper:3.4.6 (*) | | | \--- org.fusesource.leveldbjni:leveldbjni-all:1.8 | | +--- com.google.protobuf:protobuf-java:2.5.0 | | \--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 | +--- org.apache.hadoop:hadoop-mapreduce-client-shuffle:2.7.3 | | +--- org.apache.hadoop:hadoop-yarn-server-common:2.7.3 (*) | | +--- org.apache.hadoop:hadoop-yarn-server-nodemanager:2.7.3 | | | +--- org.apache.hadoop:hadoop-yarn-common:2.7.3 (*) | | | +--- org.apache.hadoop:hadoop-yarn-api:2.7.3 (*) | | | +--- javax.xml.bind:jaxb-api:2.2.2 (*) | | | +--- org.codehaus.jettison:jettison:1.1 | | | +--- commons-lang:commons-lang:2.6 | | | +--- javax.servlet:servlet-api:2.5 | | | +--- commons-codec:commons-codec:1.4 -> 1.9 | | | +--- com.sun.jersey:jersey-core:1.9 | | | +--- com.sun.jersey:jersey-client:1.9 (*) | | | +--- org.mortbay.jetty:jetty-util:6.1.26 | | | +--- com.google.guava:guava:11.0.2 -> 29.0-jre (*) | | | +--- commons-logging:commons-logging:1.1.3 -> 1.2 | | | +--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 | | | +--- com.google.protobuf:protobuf-java:2.5.0 | | | +--- com.google.inject:guice:3.0 (*) | | | +--- com.sun.jersey:jersey-json:1.9 (*) | | | +--- com.sun.jersey.contribs:jersey-guice:1.9 (*) | | | +--- org.apache.hadoop:hadoop-yarn-server-common:2.7.3 (*) | | | \--- org.fusesource.leveldbjni:leveldbjni-all:1.8 | | +--- org.apache.hadoop:hadoop-mapreduce-client-common:2.7.3 (*) | | +--- org.fusesource.leveldbjni:leveldbjni-all:1.8 | | +--- com.google.protobuf:protobuf-java:2.5.0 | | \--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 | +--- com.google.protobuf:protobuf-java:2.5.0 | \--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 +--- org.apache.hadoop:hadoop-yarn-api:2.7.3 (*) +--- org.apache.hadoop:hadoop-mapreduce-client-core:2.7.3 (*) +--- org.apache.hadoop:hadoop-mapreduce-client-jobclient:2.7.3 | +--- org.apache.hadoop:hadoop-mapreduce-client-common:2.7.3 (*) | +--- org.apache.hadoop:hadoop-mapreduce-client-shuffle:2.7.3 (*) | +--- com.google.protobuf:protobuf-java:2.5.0 | \--- org.slf4j:slf4j-api:1.7.10 -> 2.0.7 \--- org.apache.hadoop:hadoop-annotations:2.7.3

seems only hadoop-hdfs, hadoop-common are needed.

xunliu · 2023-09-24T10:57:17Z

integration-test/src/main/java/com/datastrato/graviton/integration/test/MiniGraviton.java

+    configMap.put(
+        AuxiliaryServiceManager.GRAVITON_AUX_SERVICE_PREFIX
+            + AuxiliaryServiceManager.AUX_SERVICE_NAMES,
+        IcebergAuxiliaryService.SERVICE_NAME);
+
+    String icebergClassPath = Paths.get("catalog-lakehouse", "build", "libs").toString();
+    configMap.put(
+        AuxiliaryServiceManager.GRAVITON_AUX_SERVICE_PREFIX
+            + IcebergAuxiliaryService.SERVICE_NAME
+            + "."
+            + AuxiliaryServiceManager.AUX_SERVICE_CLASSPATH,
+        icebergClassPath);
+    configMap.put(
+        AuxiliaryServiceManager.GRAVITON_AUX_SERVICE_PREFIX
+            + IcebergAuxiliaryService.SERVICE_NAME
+            + "."
+            + IcebergRESTConfig.ICEBERG_REST_SERVER_HTTP_PORT.getKey(),
+        String.valueOf(RESTUtils.findAvailablePort(3000, 4000)));
+    configMap.put(
+        AuxiliaryServiceManager.GRAVITON_AUX_SERVICE_PREFIX
+            + IcebergAuxiliaryService.SERVICE_NAME
+            + "."
+            + IcebergRESTConfig.CATALOG_IMPL.getKey(),
+        "hive");


These codes are only used for catalog-Iceberg, I think, we may need to move these codes to CatalogIcebergIT.

I think we can add a customizeCatalogConfig() interface in the customizeConfigFile() function. different catalog through customizeCatalogConfig() set different config.

good idea, but startIntegrationTest is static which means all tests share one MiniGraviton, could we start sevral MiniGravtion servers concurrently? the logic is implemented in #427, let's move there

FANNG1 · 2023-09-24T15:39:50Z

this pr contains the #427 , and is not ready to review, maybe we could change the status to Draft, and change the title to something like Support Iceberg HiveCatalog&JDBCCatalog

yuqi1129 · 2023-09-25T01:58:23Z

core/src/main/java/com/datastrato/graviton/Config.java

   */
-  private void loadFromMap(Map<String, String> map) {
+  public void loadFromMap(Map<String, String> map, boolean checkPrefix) {


I would suggest you replace the checkPrefix with Predicate to make the function more generic
like

Suggested change

public void loadFromMap(Map<String, String> map, boolean checkPrefix) {

public void loadFromMap(Map<String, String> map, Predicate<String> condition) ;

By the way, I remember @sandflee has already added a method with the same function, you can contact him to confirm.

yuqi1129 · 2023-09-25T11:05:35Z

...src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergAuxiliaryService.java

+
+public class IcebergAuxiliaryService implements GravitonAuxiliaryService {
+
+  Logger LOG = LoggerFactory.getLogger(IcebergAuxiliaryService.class);


private static final?

yuqi1129 · 2023-09-26T11:53:56Z

...house/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergRESTConfig.java

+  public static final ConfigEntry<String> CATALOG_IMPL =
+      new ConfigBuilder("catalogImpl")
+          .doc("Choose the implementation of the iceberg catalog")
+          .version("0.1.0")


0.1.0 --> 0.2.0 and this also applied in line 16.

yuqi1129 · 2023-09-26T11:58:28Z

...src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergAuxiliaryService.java

+    // todo, use JettyServer when it's moved to graviton common package
+    int port = restConfig.get(IcebergRESTConfig.ICEBERG_REST_SERVER_HTTP_PORT);
+    LOG.info("Iceberg REST service http port:{}", port);
+    Server server = new Server(port);


You'd better change the name of server here, because it will hide the field server in class IcebergAuxiliaryService

yuqi1129 · 2023-09-26T11:59:13Z

...src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergAuxiliaryService.java

+    IcebergRESTConfig icebergConfig = new IcebergRESTConfig();
+    icebergConfig.loadFromMap(properties, false);
+    server = initServer(icebergConfig);
+    LOG.info("Iceberg REST service inited");


It's not advised to use short-name inited

yuqi1129 · 2023-09-26T12:01:36Z

...house/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergRESTConfig.java

+          .doc("Choose the implementation of the iceberg catalog")
+          .version("0.1.0")
+          .stringConf()
+          .createWithDefault("memory");


Choose the implementation of the iceberg catalog

As the default value is memory, you mean all iceberg catalog data will be stored in memory, I not very clear about the value of memory.

FANNG1 · 2023-09-26T12:09:01Z

@yunqing-wei , could we close this pr ? since this's mainly for test and some person still reviews it.

Clearvive force-pushed the rest branch 6 times, most recently from 3b3ee02 to fc0057c Compare September 19, 2023 11:48

Clearvive force-pushed the rest branch 3 times, most recently from d41acfb to e0ad958 Compare September 20, 2023 02:08

Clearvive closed this Sep 20, 2023

Clearvive force-pushed the rest branch from e0ad958 to 4c66ab6 Compare September 20, 2023 02:17

Clearvive reopened this Sep 20, 2023

Clearvive marked this pull request as ready for review September 20, 2023 02:18

FANNG1 and others added 2 commits September 20, 2023 18:49

add iceberg rest service

659449e

[#411] test: support E2E tests for IcebergREST server

6d98e1e

Clearvive force-pushed the rest branch from ee738e8 to 6d98e1e Compare September 21, 2023 03:12

[#411] test: support E2E tests for IcebergREST server

6be2d4f

xunliu reviewed Sep 24, 2023

View reviewed changes

yuqi1129 reviewed Sep 25, 2023

View reviewed changes

yuqi1129 reviewed Sep 26, 2023

View reviewed changes

Clearvive self-assigned this Oct 9, 2023

Clearvive added this to the Graviton v0.2.0 milestone Oct 9, 2023

Clearvive closed this Oct 12, 2023

Clearvive deleted the rest branch October 27, 2023 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#411] test: support E2E tests for IcebergREST server #416

[#411] test: support E2E tests for IcebergREST server #416

Clearvive commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

xunliu Sep 24, 2023 •

edited

Loading

FANNG1 Sep 25, 2023

xunliu Sep 24, 2023 •

edited

Loading

FANNG1 Sep 24, 2023

xunliu Sep 24, 2023

FANNG1 Sep 25, 2023 •

edited

Loading

FANNG1 commented Sep 24, 2023 •

edited

Loading

yuqi1129 Sep 25, 2023

yuqi1129 Sep 25, 2023

yuqi1129 Sep 25, 2023

yuqi1129 Sep 26, 2023

yuqi1129 Sep 26, 2023

yuqi1129 Sep 26, 2023

yuqi1129 Sep 26, 2023

FANNG1 commented Sep 26, 2023

	public void loadFromMap(Map<String, String> map, boolean checkPrefix) {
	public void loadFromMap(Map<String, String> map, Predicate<String> condition) ;


		public class IcebergAuxiliaryService implements GravitonAuxiliaryService {

		Logger LOG = LoggerFactory.getLogger(IcebergAuxiliaryService.class);

[#411] test: support E2E tests for IcebergREST server #416

[#411] test: support E2E tests for IcebergREST server #416

Conversation

Clearvive commented Sep 19, 2023

What changes were proposed in this pull request?

github-actions bot commented Sep 19, 2023 • edited Loading

Code Coverage Report

xunliu Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

FANNG1 commented Sep 26, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

xunliu Sep 24, 2023 •

edited

Loading

xunliu Sep 24, 2023 •

edited

Loading

FANNG1 Sep 25, 2023 •

edited

Loading

FANNG1 commented Sep 24, 2023 •

edited

Loading