You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/10/18 06:28:36 UTC
[GitHub] [calcite] tgrall opened a new pull request #2223: Proposal : 4344 test container sample
tgrall opened a new pull request #2223:
URL: https://github.com/apache/calcite/pull/2223
This is a proposal based on the Redis adapter to start to use https://www.testcontainers.org/ to avoid maintaining "fake server"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r507085257
##########
File path: redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java
##########
@@ -16,25 +16,90 @@
*/
package org.apache.calcite.adapter.redis;
+import org.apache.calcite.config.CalciteSystemProperty;
+
import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.testcontainers.DockerClientFactory;
import org.testcontainers.containers.GenericContainer;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.util.logging.Logger;
+
+import redis.embedded.RedisServer;
+
/**
* RedisITCaseBase.
*/
@Execution(ExecutionMode.SAME_THREAD)
public abstract class RedisCaseBase {
+ public static final int PORT = getAvailablePort();
+ public static final String HOST = "127.0.0.1";
+ private static final String MAX_HEAP = "maxheap 51200000";
+ // If Docker is running, a container will be used
public static final GenericContainer<?> REDIS_CONTAINER =
- new GenericContainer<>("redis:6.0.6").withExposedPorts(6379);
+ new GenericContainer<>("redis").withExposedPorts(6379);
+
+ // If docker is not running use a fake server
+ private static RedisServer redisServer;
@BeforeAll
public static void startRedisContainer() {
- REDIS_CONTAINER.start();
+ // Check if docker is running, and start container if possible
+ if (CalciteSystemProperty.TEST_WITH_DOCKER_CONTAINER.value()
+ && DockerClientFactory.instance().isDockerAvailable()) {
+ REDIS_CONTAINER.start();
+ }
+ }
+
+ @BeforeEach
+ public void createRedisServer() throws IOException {
+ if (!REDIS_CONTAINER.isRunning()) {
+ if (isWindows()) {
+ redisServer = RedisServer.builder().port(PORT).setting(MAX_HEAP).build();
Review comment:
Can you please clarify why is this branching required?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tgrall commented on pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
tgrall commented on pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#issuecomment-711149463
During tests:
* Redis adapter is using a docker container if docker is running on the development machine
* If docker is not running (taking some) the Redis use the MiniRedisServer (fake Redis instead)
* it is possible to force the test to NOT use Docker or not try to use Redis `./gradlew cleanTest test -Dcalcite.test.docker=false`
This means:
* When you are running a machine with docker you test with the real product
* In CI, since Docker is available it is tested with the real product
* So far it is only available for Redis, may be interesting to reuse.
Note: it is a different approach than CALCITE-4278 that uses docker/docker-compose for CI test bu not in development. The approach here is to also use container in development when possible, and CI too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#issuecomment-711175458
@zabetak , it looks like https://twitter.com/rootsecdev/status/1317485938507919360, doesn't it? :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r515817104
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -239,6 +239,12 @@
public static final CalciteSystemProperty<Boolean> TEST_REDIS =
booleanProperty("calcite.test.redis", true);
+ /**
+ * Whether to use Docker with TestContainers (https://www.testcontainers.org/) when available
+ */
+ public static final CalciteSystemProperty<Boolean> TEST_WITH_DOCKER_CONTAINER =
+ booleanProperty("calcite.test.docker", true);
+
Review comment:
What's the benefit of having this property?
If Docker is available we can run the tests, if not then either we skip them or fallback to another execution method.
##########
File path: redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java
##########
@@ -36,16 +43,33 @@
public static final String HOST = "127.0.0.1";
private static final String MAX_HEAP = "maxheap 51200000";
+ // If Docker is running, a container will be used
+ public static final GenericContainer<?> REDIS_CONTAINER =
+ new GenericContainer<>("redis").withExposedPorts(6379);
Review comment:
I agree, let's freeze the version ideally using the same that is present in the gradle.properties.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2223:
URL: https://github.com/apache/calcite/pull/2223
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tgrall commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
tgrall commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r507090672
##########
File path: redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java
##########
@@ -16,25 +16,90 @@
*/
package org.apache.calcite.adapter.redis;
+import org.apache.calcite.config.CalciteSystemProperty;
+
import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.testcontainers.DockerClientFactory;
import org.testcontainers.containers.GenericContainer;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.util.logging.Logger;
+
+import redis.embedded.RedisServer;
+
/**
* RedisITCaseBase.
*/
@Execution(ExecutionMode.SAME_THREAD)
public abstract class RedisCaseBase {
+ public static final int PORT = getAvailablePort();
+ public static final String HOST = "127.0.0.1";
+ private static final String MAX_HEAP = "maxheap 51200000";
+ // If Docker is running, a container will be used
public static final GenericContainer<?> REDIS_CONTAINER =
- new GenericContainer<>("redis:6.0.6").withExposedPorts(6379);
+ new GenericContainer<>("redis").withExposedPorts(6379);
+
+ // If docker is not running use a fake server
+ private static RedisServer redisServer;
@BeforeAll
public static void startRedisContainer() {
- REDIS_CONTAINER.start();
+ // Check if docker is running, and start container if possible
+ if (CalciteSystemProperty.TEST_WITH_DOCKER_CONTAINER.value()
+ && DockerClientFactory.instance().isDockerAvailable()) {
+ REDIS_CONTAINER.start();
+ }
+ }
+
+ @BeforeEach
+ public void createRedisServer() throws IOException {
+ if (!REDIS_CONTAINER.isRunning()) {
+ if (isWindows()) {
+ redisServer = RedisServer.builder().port(PORT).setting(MAX_HEAP).build();
Review comment:
No I cannot clarify ;) - I just restored the initial code to run the RedisMiniServer and allow the test to run either with Docker or with MiniServer.
See current [master](https://github.com/apache/calcite/blob/bda7fb188b01c0334a7bf1662101e259321893a4/redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java#L43-L47) _(I have not tested on Windows)_
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r515826195
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -239,6 +239,12 @@
public static final CalciteSystemProperty<Boolean> TEST_REDIS =
booleanProperty("calcite.test.redis", true);
+ /**
+ * Whether to use Docker with TestContainers (https://www.testcontainers.org/) when available
+ */
+ public static final CalciteSystemProperty<Boolean> TEST_WITH_DOCKER_CONTAINER =
+ booleanProperty("calcite.test.docker", true);
+
Review comment:
@tgrall , would you please add a corresponding comment to the javadoc?
I believe the key point of having such a property is to allow running a non-docker test even in the case Docker is installed on the developer's machine.
For instance, it might be helpful to replicate an issue that happens only in non-docker test mode.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bsideup commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
bsideup commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r507705443
##########
File path: redis/src/test/java/org/apache/calcite/adapter/redis/RedisCaseBase.java
##########
@@ -36,16 +43,33 @@
public static final String HOST = "127.0.0.1";
private static final String MAX_HEAP = "maxheap 51200000";
+ // If Docker is running, a container will be used
+ public static final GenericContainer<?> REDIS_CONTAINER =
+ new GenericContainer<>("redis").withExposedPorts(6379);
Review comment:
consider using a fixed version image, for reproducible tests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#issuecomment-735662405
Although we didn't heard back from @tgrall it seems that there only few minor things left to take care of. If nobody objects, I will treat those and merge this PR in the following days.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tgrall commented on pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
tgrall commented on pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#issuecomment-735663207
Hello.
Sorry for the late reply, I am totally swamped with other tasks for now.
On Mon, Nov 30, 2020 at 10:21 AM Stamatis Zampetakis <
notifications@github.com> wrote:
> Although we didn't heard back from @tgrall <https://github.com/tgrall> it
> seems that there only few minor things left to take care of. If nobody
> objects, I will treat those and merge this PR in the following days.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/calcite/pull/2223#issuecomment-735662405>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAEEEQUQ2PKZDMQRNCV2USDSSNPYDANCNFSM4SU35LKQ>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bsideup commented on a change in pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
bsideup commented on a change in pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#discussion_r507705897
##########
File path: redis/src/test/java/org/apache/calcite/adapter/redis/RedisAdapterCaseBase.java
##########
@@ -103,7 +103,7 @@ private void readModelByJson() {
if (file.exists()) {
JsonNode rootNode = objMapper.readTree(file);
strResult = rootNode.toString().replace(Integer.toString(Protocol.DEFAULT_PORT),
- Integer.toString(PORT));
+ Integer.toString(getRedisServerPort()));
Review comment:
make sure that the host is also dynamic
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #2223: Proposal : 4344 test container sample
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #2223:
URL: https://github.com/apache/calcite/pull/2223#issuecomment-711173315
I like the approach :)
Just to clarify CALCITE-4278 does not run out-of-the-box in dev mode only cause I didn't have time to fill in the missing parts; it will be nice to change this in the future!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org