yuqi1129 commented on code in PR #4005: URL: https://github.com/apache/gravitino/pull/4005#discussion_r1667929808
########## gradle.properties: ########## @@ -40,4 +40,4 @@ defaultScalaVersion = 2.12 pythonVersion = 3.8 # skipDockerTests is used to skip the tests that require Docker to be running. -skipDockerTests = true Review Comment: Please revert the changes here. ########## iceberg-rest-server/src/test/resources/log4j2.properties: ########## @@ -0,0 +1,73 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +# Set to debug or trace if log4j initialization is failing +status = info + +# Name of the configuration +name = ConsoleLogConfig + +# Console appender configuration +appender.console.type = Console +appender.console.name = consoleLogger +appender.console.layout.type = PatternLayout +appender.console.layout.pattern = %d{yyyy-MM-dd HH:mm:ss} %-5p [%t] %c{1}:%L - %m%n + +# Log files location +property.logPath = ${sys:gravitino.log.path:-iceberg-rest-server/build/iceberg-integration-test.log} Review Comment: Remove `iceberg-rest-server/` as it will create a new folder `iceberg-rest-server` under the project `iceberg-rest-server` ########## iceberg-rest-server/src/test/java/com/datastrato/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.datastrato.gravitino.iceberg.integration.test.util; + +import static com.google.common.util.concurrent.Uninterruptibles.sleepUninterruptibly; + +import com.datastrato.gravitino.Config; +import com.datastrato.gravitino.iceberg.common.IcebergConfig; +import com.datastrato.gravitino.integration.test.util.ITUtils; +import com.datastrato.gravitino.rest.RESTUtils; +import com.datastrato.gravitino.server.IcebergRESTServer; +import com.datastrato.gravitino.server.ServerConfig; +import com.datastrato.gravitino.server.web.JettyServerConfig; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.classic.methods.HttpGet; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.HttpClients; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.core5.http.ClassicHttpResponse; +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.Properties; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public abstract class IcebergRESTServerManager { + + protected static final Logger LOG = LoggerFactory.getLogger(IcebergRESTServerManager.class); + + protected Map<String, String> customConfigs = new HashMap<>(); + protected Config serverConfig; + protected String checkUri; + + public abstract Path getConfigDir(); + + public abstract Optional<Future> doStartIcebergRESTServer() throws Exception; + + public abstract void doStopIcebergRESTServer(); + + public static IcebergRESTServerManager create() { + String testMode = + System.getProperty(com.datastrato.gravitino.integration.test.util.ITUtils.TEST_MODE); + if (com.datastrato.gravitino.integration.test.util.ITUtils.EMBEDDED_TEST_MODE.equals( + testMode)) { + return new IcebergRESTServerManagerForEmbedded(); + } else { + return new IcebergRESTServerManagerForDeploy(); + } + } + + public void registerCustomConfigs(Map<String, String> configs) { + customConfigs.putAll(configs); + } + + public Config getServerConfig() { + return serverConfig; + } + + public void startIcebergRESTServer() throws Exception { + initServerConfig(); + Optional<Future> future = doStartIcebergRESTServer(); + + long beginTime = System.currentTimeMillis(); + boolean started = false; + + while (System.currentTimeMillis() - beginTime < 1000 * 60) { + started = isHttpServerUp(checkUri); + if (started || (future.isPresent() && future.get().isDone())) { + break; + } + sleepUninterruptibly(500, TimeUnit.MILLISECONDS); + } + if (!started) { + try { + if (future.isPresent()) { + future.get().get(1, TimeUnit.SECONDS); + } + } catch (Exception e) { + throw new RuntimeException("IcebergRESTServer start failed", e); + } + throw new RuntimeException("Can not start IcebergRESTServer"); + } + } + + public void stopIcebergRESTServer() { + doStopIcebergRESTServer(); + sleepUninterruptibly(500, TimeUnit.MILLISECONDS); + + long beginTime = System.currentTimeMillis(); + boolean started = true; + while (System.currentTimeMillis() - beginTime < 3 * 1000 * 60) { + sleepUninterruptibly(500, TimeUnit.MILLISECONDS); + started = isHttpServerUp(checkUri); + if (!started) { + break; + } + } + if (started) { + throw new RuntimeException("Can not stop IcebergRESTServer"); + } + } + + private static boolean isHttpServerUp(String testUrl) { + try (CloseableHttpClient httpClient = HttpClients.createDefault()) { + HttpGet request = new HttpGet(testUrl); + ClassicHttpResponse response = httpClient.execute(request, a -> a); + return response.getCode() == 200; + } catch (Exception e) { + LOG.warn("Check IcebergRESTServer failed, url:{}, error:{} ", testUrl, e.getMessage()); Review Comment: Since the method name is `isHttpServerUp`, the error messages here should be not `IcebergRESTServer`. ########## iceberg-rest-server/src/test/java/com/datastrato/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.datastrato.gravitino.iceberg.integration.test.util; + +import static com.google.common.util.concurrent.Uninterruptibles.sleepUninterruptibly; + +import com.datastrato.gravitino.Config; +import com.datastrato.gravitino.iceberg.common.IcebergConfig; +import com.datastrato.gravitino.integration.test.util.ITUtils; +import com.datastrato.gravitino.rest.RESTUtils; +import com.datastrato.gravitino.server.IcebergRESTServer; +import com.datastrato.gravitino.server.ServerConfig; +import com.datastrato.gravitino.server.web.JettyServerConfig; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.classic.methods.HttpGet; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.HttpClients; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.core5.http.ClassicHttpResponse; +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.Properties; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public abstract class IcebergRESTServerManager { + + protected static final Logger LOG = LoggerFactory.getLogger(IcebergRESTServerManager.class); + + protected Map<String, String> customConfigs = new HashMap<>(); + protected Config serverConfig; + protected String checkUri; + + public abstract Path getConfigDir(); + + public abstract Optional<Future> doStartIcebergRESTServer() throws Exception; Review Comment: Future -> Future<?> ########## iceberg-rest-server/src/test/java/com/datastrato/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.datastrato.gravitino.iceberg.integration.test.util; + +import static com.google.common.util.concurrent.Uninterruptibles.sleepUninterruptibly; + +import com.datastrato.gravitino.Config; +import com.datastrato.gravitino.iceberg.common.IcebergConfig; +import com.datastrato.gravitino.integration.test.util.ITUtils; +import com.datastrato.gravitino.rest.RESTUtils; +import com.datastrato.gravitino.server.IcebergRESTServer; +import com.datastrato.gravitino.server.ServerConfig; +import com.datastrato.gravitino.server.web.JettyServerConfig; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.classic.methods.HttpGet; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.impl.classic.HttpClients; +import com.github.dockerjava.zerodep.shaded.org.apache.hc.core5.http.ClassicHttpResponse; +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.Properties; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public abstract class IcebergRESTServerManager { + + protected static final Logger LOG = LoggerFactory.getLogger(IcebergRESTServerManager.class); + + protected Map<String, String> customConfigs = new HashMap<>(); + protected Config serverConfig; + protected String checkUri; + + public abstract Path getConfigDir(); + + public abstract Optional<Future> doStartIcebergRESTServer() throws Exception; + + public abstract void doStopIcebergRESTServer(); + + public static IcebergRESTServerManager create() { + String testMode = + System.getProperty(com.datastrato.gravitino.integration.test.util.ITUtils.TEST_MODE); + if (com.datastrato.gravitino.integration.test.util.ITUtils.EMBEDDED_TEST_MODE.equals( Review Comment: Please shorten the class name `com.datastrato.gravitino.integration.test.util.ITUtils` ########## core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java: ########## @@ -120,25 +120,29 @@ public static GravitinoEnv getInstance() { * * @param config The configuration object to initialize the environment. */ - public void initialize(Config config) { + public void initialize(Config config, boolean initGravitinoServerComponet) { Review Comment: Typo: initGravitinoServerComponet ########## conf/gravitino.conf.template: ########## @@ -63,9 +63,12 @@ gravitino.catalog.cache.evictionIntervalMs = 3600000 # Auxiliary service names, separate by ',' gravitino.auxService.names = iceberg-rest # Iceberg REST service classpath -gravitino.auxService.iceberg-rest.classpath = catalogs/lakehouse-iceberg/libs, catalogs/lakehouse-iceberg/conf +gravitino.iceberg-rest.classpath = extensions/iceberg-rest-server/libs # Iceberg REST service host -gravitino.auxService.iceberg-rest.host = 0.0.0.0 +gravitino.iceberg-rest.host = 0.0.0.0 # Iceberg REST service http port -gravitino.auxService.iceberg-rest.httpPort = 9001 - +gravitino.iceberg-rest.httpPort = 9001 +# The backend Iceberg catalog for Iceberg REST service, it's recommanded to change to hive or jdbc +gravitino.iceberg-rest.catalog-backend = memory +# The warehouse directory of Iceberg catalog for Iceberg REST service +gravitino.iceberg-rest.warehouse = /tmp/ Review Comment: Why do we need to add `gravitino.iceberg-rest.warehouse` in `gravitino.conf` and `iceberg-rest-server` both? ########## .github/workflows/backend-integration-test.yml: ########## @@ -37,6 +37,8 @@ jobs: - gradle/** - integration-test/** - integration-test-common/** + - iceberg-common/** Review Comment: It seems to not very proper to put common logic for Iceberg catalog under the root project here, I think  -- 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]
