feat: add oceanbase-ce module #7502

whhe · 2023-09-06T06:56:37Z

OceanBase is a distributed relational database developed by Ant Group, and its community edition is open sourced at https://github.com/oceanbase/oceanbase.

There is a docker image maintained by the official team, and in this pull request I add OceanBaseContainer and OceanBaseContainerProvider for a new oceanbase-ce module based on the docker image. https://github.com/oceanbase/oceanbase/tree/master/tools/docker/standalone.

eddumelendez

Thanks for your contribution! I have left some suggestions. Please, also take a look at at the build which just failed.

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

whhe · 2023-09-07T03:11:34Z

Thanks for your contribution! I have left some suggestions. Please, also take a look at at the build which just failed.

Thank you for your review, I have addressed the comments in the latest commit.

For the CI failure, it seems the container start failed, and I guess the reason may be the memory is not enought. To start the docker container of OceanBase, at least 2 cpu and 8g memory are required.

eddumelendez · 2023-09-07T05:10:47Z

To start the docker container of OceanBase, at least 2 cpu and 8g memory are required.

I think this is a concern due to IIRC Docker Desktop provides a default of 4g or 8g so it will be at limit all the time not even allowing running other test in parallel. Is there any way to consume less resources. So far, I've seen DB2 consuming ~1.75g

eddumelendez

Thanks for addressing the initial comments. Is there a web console provided with the image? Also it would be great if the image provides a directory where scripts can be copied and be part of the database initialization such as mysql does it due to the withInitScript limitation

docs/modules/databases/oceanbasece.md

modules/oceanbase-ce/src/test/java/org/testcontainers/junit/oceanbase/SimpleOceanBaseTest.java

whhe · 2023-09-07T05:31:42Z

To start the docker container of OceanBase, at least 2 cpu and 8g memory are required.

I think this is a concern due to IIRC Docker Desktop provides a default of 4g or 8g so it will be at limit all the time not even allowing running other test in parallel. Is there any way to consume less resources. So far, I've seen DB2 consuming ~1.75g

We have a plan to achieve lightweight deployment, but it may be difficult to achieve it in the near future.

Now we can't deploy the container on a free GitHub agent. For reference, we use the docker image through testcontainers on a self-hosted CI service in flink cdc.

eddumelendez · 2023-09-07T05:36:45Z

Now we can't deploy the container on a free GitHub agent.

TBH, I think for now it will keep on hold if that the case 😕

Another option is to self-host the module as others do.

whhe · 2023-09-07T06:00:17Z

Now we can't deploy the container on a free GitHub agent.

TBH, I think for now it will keep on hold if that the case 😕

Another option is to self-host the module as others do.

OK, I understand. I would convert this pull request to a draft, and we can continue the process after oceanbase/oceanbase#1557 is basically completed.

eddumelendez · 2023-09-07T06:09:51Z

great! Thanks for your understanding! I'll be following the issue in order to get updates

whhe · 2023-09-20T08:50:46Z

I updated the container class with the new image oceanbase/oceanbase-ce:4.2.0.0 which can be deployed on GitHub Action now https://hub.docker.com/r/oceanbase/oceanbase-ce.

Currently the startup time is still more than 1 minute, I think we can use 3 min in this version and update it when oceanbase/oceanbase#1573 is released. WDYT? @eddumelendez

eddumelendez · 2023-09-25T14:59:24Z

Hi @whhe, great to see the improvements. Currently, 1 min is the maximum value we are setting. I would say reaching that limit would produce a not good experience when running tests, of course, it depends how those are set too. IMO, we can wait a little bit more and provide the best experience possible when the other issue is fixed.

whhe · 2023-09-26T03:02:51Z

Hi @whhe, great to see the improvements. Currently, 1 min is the maximum value we are setting. I would say reaching that limit would produce a not good experience when running tests, of course, it depends how those are set too. IMO, we can wait a little bit more and provide the best experience possible when the other issue is fixed.

Thanks for the suggestion, I think it makes perfect sense, will update this PR with the new image later.

whhe · 2024-01-05T09:24:35Z

Hi @eddumelendez, I updated this PR with our latest docker image, which can start in less than 30s with FASTBOOT enabled. I think we can go back to the review process now.

whhe · 2024-01-24T06:54:58Z

@eddumelendez PTAL if you are available.

whhe · 2024-02-01T03:30:54Z

Hi @eddumelendez, is there any plan for this pr?

eddumelendez

Thanks for your contribution, @whhe ! and sorry for the late reply. I have left some comments and some more general:

Rename the module to oceanbase. Is it ok for you?
Package should be org.testcontainers.oceanbase
Run ./gradlew spotlessApply

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

modules/oceanbase-ce/src/test/java/org/testcontainers/OceanBaseTestImages.java

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

eddumelendez · 2024-02-08T04:27:25Z

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

+        if (StringUtils.isEmpty(tenantName)) {
+            throw new IllegalArgumentException("Tenant name cannot be null or empty");
+        }
+        if (SYSTEM_TENANT_NAME.equals(tenantName)) {
+            throw new IllegalArgumentException("Tenant name cannot be " + SYSTEM_TENANT_NAME);
+        }


wonder if those validations should be at the image level and avoid duplicating the validation.

The response from the image may not be as direct as the error message here. It refers to the 'DEFAULT_DATABASE_NAME' in OracleContainer.

eddumelendez · 2024-02-08T04:30:32Z

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

+
+    public String getJdbcUrl(String databaseName) {
+        String additionalUrlParams = constructUrlParameters("?", "&");
+        String prefix = driverClassName.contains("mysql") ? "jdbc:mysql://" : "jdbc:oceanbase://";


I think we should restrict more here because at first glance if any driverClassName different than mysql will choose oceanbase prefix, and not sure but that would be unexpected.

The 'driverClassName' is verified in withDriverClassName method, so its value is limited to these two options.

modules/oceanbase-ce/src/main/java/org/testcontainers/containers/OceanBaseContainer.java

whhe · 2024-02-18T02:15:18Z

Thanks for your contribution, @whhe ! and sorry for the late reply. I have left some comments and some more general:

Rename the module to oceanbase. Is it ok for you?

Package should be org.testcontainers.oceanbase

Run ./gradlew spotlessApply

Thanks for your review and sorry for the late reply, I just came back from the Chinese new year holiday.

I will address the comments soon, but for the module name, I used 'oceanbase-ce' because the community will most likely release a 'oceanbase-xe' docker image in the future for the OceanBase Enterprise Edition. So I think we'd better keep it here.

eddumelendez · 2024-02-18T04:06:42Z

I think in that case the module can still be oceanbase and contain two implementations OceanbaseCEContainer and OceanbaseXEContainer, it depends in how much difference there is between both of them.

whhe · 2024-02-22T06:19:54Z

@eddumelendez PR updated, PTAL.

whhe · 2024-02-26T03:27:46Z

@eddumelendez Look forward to your feedback.

eddumelendez

Thanks for the quick update! Left a couple of messages.

eddumelendez · 2024-02-26T19:42:10Z

modules/oceanbase/src/main/java/org/testcontainers/oceanbase/OceanBaseCEContainer.java

+    @Override
+    protected void configure() {
+        if (!DEFAULT_TEST_TENANT_NAME.equals(tenantName)) {
+            withEnv("OB_TENANT_NAME", tenantName);
+        }
+    }


according to the docs, the default value for OB_TENAN_NAME is test. So we can just have

Suggested change

@Override

protected void configure() {

if (!DEFAULT_TEST_TENANT_NAME.equals(tenantName)) {

withEnv("OB_TENANT_NAME", tenantName);

}

}

@Override

protected void configure() {

withEnv("OB_TENANT_NAME", tenantName);

}

User can also use 'withEnv' method to set it manually, in this case if we do not check the value, there may be a mistake.

I don't fully understand because the default is test and the tenantName = DEFAULT_TEST_TENANT_NAME; where DEFAULT_TEST_TENANT_NAME = "test". Unless, the right one should use SYSTEM_TENANT_NAME instead

if (!SYSTEM_TENANT_NAME.equals(tenantName)) { withEnv("OB_TENANT_NAME", tenantName); }

Once we resolve this comment I can proceed and merge it

I'm considering this situation: someone use the container class but not set the tenant name with given method withTenant, like new OceanBaseCEContainer("xxx").withEnv("OB_TENANT_NAME", "tc");. In this case, as the tenantName field is still 'test', a 'test' tenant will be created, not user defined value 'tc'.

I'm not sure if we should deal with it. WDYT?

I think that given there is a default value in the image itself. The withTenant method and the withEnv in configure method can be omitted. If for some reason there is a need to modify it then new OceanBaseCEContainer(...).withEnv("OB_TENANT_NAME", "something"); that would a similar experience like running docker run command. Otherwise, it would be a little bit confusing.

The tenantName field here is required to construct the username used in jdbc connection, so I did not remove the withTenant method in previous cleanup. I reconsidered about it and maybe use the default value with necessary description is better.

I just noticed we were missing a test for this use case and also think a little bit more about it. In the configure method we can add getEnvMap().computeIfAbsent("OB_TENAN_NAME", k -> tenantName != null ? tenantName : DEFAULT_TENANT_NAME) or just getEnvMap().computeIfAbsent("OB_TENAN_NAME", k -> tenantName) and we can keep withTenantName. LMK if this can help to the use case described.

Yes you are right, we can resolve it by using the envMap, but I think it's not necessary for now.

At present no other withXXX method is provided in this container class and all env options are set by withEnv, I think it's good and clean. Lets keep it like this.

modules/oceanbase/src/main/java/org/testcontainers/oceanbase/OceanBaseCEContainer.java

eddumelendez · 2024-02-27T23:28:40Z

One last change, can we add an entry for oceanbase here, please?

modules/oceanbase/src/main/java/org/testcontainers/oceanbase/OceanBaseCEContainer.java

modules/oceanbase/src/test/java/org/testcontainers/junit/oceanbase/SimpleOceanBaseCETest.java

modules/oceanbase/src/test/java/org/testcontainers/jdbc/oceanbase/OceanBaseJdbcDriverTest.java

modules/oceanbase/src/main/java/org/testcontainers/oceanbase/OceanBaseCEContainer.java

eddumelendez · 2024-02-28T23:59:46Z

Thanks for your contribution, @whhe !

whhe requested a review from a team as a code owner September 6, 2023 06:56

github-actions bot added the type/docs label Sep 6, 2023

eddumelendez added the type/new module label Sep 6, 2023

eddumelendez requested changes Sep 6, 2023

View reviewed changes

eddumelendez reviewed Sep 7, 2023

View reviewed changes

docs/modules/databases/oceanbasece.md Outdated Show resolved Hide resolved

modules/oceanbase-ce/src/test/java/org/testcontainers/junit/oceanbase/SimpleOceanBaseTest.java Outdated Show resolved Hide resolved

whhe marked this pull request as draft September 7, 2023 06:00

whhe force-pushed the oceanbase-ce branch from 5f0d3aa to f9e482d Compare September 20, 2023 08:25

whhe marked this pull request as ready for review September 20, 2023 08:27

whhe force-pushed the oceanbase-ce branch 2 times, most recently from fed062f to 0210dcf Compare January 5, 2024 09:20

whhe force-pushed the oceanbase-ce branch 2 times, most recently from 7011359 to d2a6a21 Compare January 16, 2024 07:23

whhe requested a review from eddumelendez January 16, 2024 07:29

feat: add oceanbase-ce module

04f4318

whhe force-pushed the oceanbase-ce branch from d2a6a21 to 04f4318 Compare January 24, 2024 06:48

eddumelendez requested changes Feb 8, 2024

View reviewed changes

whhe added 2 commits February 18, 2024 10:00

Merge branch 'main' into oceanbase-ce

d9c4384

remove getMappedPort override

76aa6f5

whhe force-pushed the oceanbase-ce branch 2 times, most recently from 2057639 to 444cdb6 Compare February 19, 2024 08:07

whhe requested a review from eddumelendez February 19, 2024 09:05

whhe force-pushed the oceanbase-ce branch from 780b88d to 26aeb9a Compare February 19, 2024 09:17

address review comments

3c2360d

whhe force-pushed the oceanbase-ce branch from 26aeb9a to 3c2360d Compare February 19, 2024 09:25

whhe added 2 commits February 22, 2024 10:45

Merge branch 'main' into oceanbase-ce

299951a

use 4.2.2 image by default

8eb7836

fix conflicts

995ab96

eddumelendez reviewed Feb 26, 2024

View reviewed changes

detect driver class before construct jdbc url

4d10383

whhe added 2 commits February 28, 2024 10:45

add oceanbase label

4c6922b

Merge branch 'main' into oceanbase-ce

e1a303f

eddumelendez reviewed Feb 28, 2024

View reviewed changes

modules/oceanbase/src/main/java/org/testcontainers/oceanbase/OceanBaseCEContainer.java Outdated Show resolved Hide resolved

remove tenant name config

66b4592

whhe force-pushed the oceanbase-ce branch from 636480c to 66b4592 Compare February 28, 2024 06:15

eddumelendez reviewed Feb 28, 2024

View reviewed changes

whhe added 2 commits February 28, 2024 14:50

modify test package name

ae13ed7

merge getJdbcUrl method

5bd35cb

eddumelendez approved these changes Feb 28, 2024

View reviewed changes

eddumelendez added this to the next milestone Feb 28, 2024

eddumelendez approved these changes Feb 28, 2024

View reviewed changes

eddumelendez merged commit f23c1ec into testcontainers:main Feb 28, 2024
96 checks passed

feat: add oceanbase-ce module #7502

feat: add oceanbase-ce module #7502

Conversation

whhe commented Sep 6, 2023

eddumelendez left a comment

Choose a reason for hiding this comment

whhe commented Sep 7, 2023

eddumelendez commented Sep 7, 2023

eddumelendez left a comment • edited

Choose a reason for hiding this comment

whhe commented Sep 7, 2023

eddumelendez commented Sep 7, 2023 • edited

whhe commented Sep 7, 2023

eddumelendez commented Sep 7, 2023

whhe commented Sep 20, 2023

eddumelendez commented Sep 25, 2023

whhe commented Sep 26, 2023

whhe commented Jan 5, 2024

whhe commented Jan 24, 2024

whhe commented Feb 1, 2024

eddumelendez left a 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

whhe commented Feb 18, 2024

eddumelendez commented Feb 18, 2024

whhe commented Feb 22, 2024

whhe commented Feb 26, 2024

eddumelendez left a 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

eddumelendez commented Feb 27, 2024

eddumelendez commented Feb 28, 2024

eddumelendez left a comment •

edited

eddumelendez commented Sep 7, 2023 •

edited