Add db2 jdbc module #860
Add db2 jdbc module #860
Conversation
|
Thanks for this! Our database coverage continues to grow! I've not looked into the licence situation yet, other than your comment above, but I think we mainly just need to divide and conquer the licence issues:
|
I kind of remembered, we have a similar situation for the other modules. I will implement the license agreement check in a similar fashion. Here is the license information regarding the official IBM image:
We are only using the (unofficial? at least unofficially hosted) JDBC driver for our own integration tests (with |
|
Dear Kevin, |
|
Yo. It´s me again, comming up with another idea:
Maybe it would also reasonable to just start the image and the user has to deal with the jdbc driver? Kind regards |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
Let's leave it open. @rnorth I still think there is some value in having this module though, I've talked with different users that very much like to use Testcontainers with DB2 (and I normally recommend them to implement this PR as their own custom container). |
|
I use the DB2 official image for testing at my firm constantly. I’d love to see this go through. |
|
If there is anything I can do to help please point me in the right direction. |
| @@ -59,6 +59,7 @@ | |||
| {"jdbc:tc:mariadb:10.2.14://hostname/databasename?TC_INITFUNCTION=org.testcontainers.jdbc.JDBCDriverTest::sampleInitFunction", EnumSet.of(Options.ScriptedSchema)}, | |||
| {"jdbc:tc:mariadb:10.2.14://hostname/databasename?TC_MY_CNF=somepath/mariadb_conf_override", EnumSet.of(Options.CustomIniFile)}, | |||
| {"jdbc:tc:clickhouse://hostname/databasename", EnumSet.of(Options.PmdKnownBroken)}, | |||
| // {"jdbc:tc:db2://hostname/databasename", EnumSet.noneOf(Options.class)}, // Test queries don't work | |||
There was a problem hiding this comment.
@rnorth Should I leave this commented, or just delete the line?
There was a problem hiding this comment.
Not sure I understand the reason why it is commented out...
| @@ -19,6 +25,7 @@ dependencies { | |||
| testCompile 'com.oracle:ojdbc6:12.1.0.1-atlassian-hosted' | |||
| testCompile 'com.microsoft.sqlserver:mssql-jdbc:6.1.0.jre8' | |||
| testCompile 'ru.yandex.clickhouse:clickhouse-jdbc:0.1.41' | |||
| testCompile 'com.ibm.db2.jcc:db2jcc4:10.1' | |||
There was a problem hiding this comment.
I'll switch this to testRuntime
| import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals; | ||
|
|
||
|
|
||
| public class SimpleDb2Test { |
There was a problem hiding this comment.
Ignore those tests by default, like Oracle
|
@rnorth Do you think we can integrate after I've resolved the parts I've commented? @sporkisfaster Thanks for the offer, I think it's basically finished from an implementation standpoint. |
|
Should this point at Developer-C instead of Express or does it matter? As far as I know the Express image is deprecated |
|
Any updates on this? Would be super helpful as I’m running into issues with genericcontainer for db2 |
|
Hi @kiview ..thank you for pointing us here in your comment on an issue (from another repo). I'm interested to understand the status. Is it the case that the issues raised in early comments have been addressed, and now it's just a matter of someone addressing the merge conflicts and completing the PR? Or, if there are other issues still remaining to work out here, could you please give an update of where we stand when you have a chance? Thank you. |
| addEnv("LICENSE", "accept"); // TODO: explicit? | ||
| addEnv("DB2INST1_PASSWORD", password); | ||
|
|
||
| withCommand("db2start"); |
There was a problem hiding this comment.
This should be set in the ctor, rather than here in configure(). Setting it here blocks any users from overriding the command (we had the same issue with the PostgreSQL module)
| protected void configure() { | ||
| addExposedPort(DB2_PORT); | ||
|
|
||
| addEnv("LICENSE", "accept"); // TODO: explicit? |
There was a problem hiding this comment.
perhaps this could be moved to an acceptLicense() method, so that the developer has to explicitly type it out and set it when they construct the container? If it's not set, we can blow up in configure() with a clear error msg indicating that the license needs to accepted via acceptLicense()
There was a problem hiding this comment.
There is LicenseAcceptance helper, used by MSSQL and Oracle, should be used here too
|
|
||
| try { | ||
| waitingConsumer.waitUntil(waitPredicate); | ||
| execInContainer("/bin/sh", "-c", "runuser -l db2inst1 -c 'db2 create db tc'"); |
There was a problem hiding this comment.
this forces the DB name to always be tc. This ought to be configurable via withDatabaseName(String)
|
|
||
| @Override | ||
| public String getJdbcUrl() { | ||
| return "jdbc:db2://" + getContainerIpAddress() + ":" + getMappedPort(DB2_PORT) + "/tc"; |
There was a problem hiding this comment.
same comment here about hard-coding the DB name to tc
|
IBM has licensed the IP of the db2 jdbc driver (jcc) to Rocket, and they have made it available in maven central https://search.maven.org/artifact/com.ibm.db2/jcc/11.1.4.4/jar. Like the db server, it is still IPLA license though. |
|
@frowe probably different thread, what about the source jar? |
|
|
The hard parts (legal and where to get the artifact on MC) seem to be figured out. All that's left now is addressing review comments and such AFAIK. |
| import java.util.concurrent.TimeoutException; | ||
| import java.util.function.Predicate; | ||
|
|
||
| public class Db2Container<SELF extends Db2Container<SELF>> extends JdbcDatabaseContainer<SELF> { |
There was a problem hiding this comment.
No need for self-typing here
| public class Db2Container<SELF extends Db2Container<SELF>> extends JdbcDatabaseContainer<SELF> { | |
| public class Db2Container extends JdbcDatabaseContainer<Db2Container> { |
|
|
||
| @Override | ||
| protected void configure() { | ||
| addExposedPort(DB2_PORT); |
There was a problem hiding this comment.
please move every static configuration to the constructor, so that it can be overridden without extending the class
|
|
||
|
|
||
| try { | ||
| waitingConsumer.waitUntil(waitPredicate); |
There was a problem hiding this comment.
please configure waitingFor. If you need to run the command after it, you can override containerStarted method instead
| @@ -172,7 +172,7 @@ private void parseUrl() { | |||
| * @author manikmagar | |||
| */ | |||
| public interface Patterns { | |||
| Pattern URL_MATCHING_PATTERN = Pattern.compile("jdbc:tc:([a-z]+)(:([^:]+))?://([^\\?]+)(\\?.*)?"); | |||
| Pattern URL_MATCHING_PATTERN = Pattern.compile("jdbc:tc:([a-z]+2?)(:([^:]+))?://([^\\?]+)(\\?.*)?"); | |||
There was a problem hiding this comment.
just add the digits range to the pattern, this 2? is too specific
Unfortunately you need to log-in and accept the license agreement in order to pull the new docker image for Developer-C ( https://hub.docker.com/_/db2-developer-c-edition ). Not sure if that's the default we want to provide. In addition that container image exposes additional configuration options. Do you have any experience with that image? On the other hand I do agree that using three year old image does not sound like a long-term solution. Upgrading the Db2Container to the new image in a later step could represent a breaking change for any clients. |
|
I've rebased the pull request, cleaned up and incorporated the feedback from the review comments to the best of my understanding and fixed the test. I could create my own pull request with the changes if someone is interested however I do not want to step on anyone's (@kiview in particular) toes. |
|
Since I'm an IBM employee I'm going to fish around a bit internally for a few days to try and get in touch with some DB2 folks and see what Docker image we should be using. Right now my preference would be going with the more current image (Developer-C image), even if it does mean accepting a license. I think people would just use an in-memory DB like H2 before they would use a 3 year old deprecated version of DB2. |
|
FWIW I've raised this ticket around the current gap in our docs for new module contributors. I'd like to have a bit of a think about the third point specifically before merging this, but otherwise I'm fine with this new module at an overall level: #1503 |
|
ok, reporting back... I anticipate a new DB2-C docker image that will not require any docker hub logins is going to be released in the near enough future that it would make sense to hold off on this PR in favor of the future DB2-C image. |
|
@aguibert thanks a lot for pushing it forward! It is so great to see a progress here, especially in such a sensitive topic as commercial DBs from giants like IBM :) If this is not the magic of OSS communities then what is? 😀 |
|
@bsideup @kiview ok, the new DB2 images are here and they no longer require any sort of docker login! https://hub.docker.com/r/ibmcom/db2 The doc is also much more clear on how to configure them. The startup time isn't great (4m on my machine), but I spoke with the DB2 team and they are have plans to reduce the startup time in the future. I'm kicking the tires with this new image w/ Testcontainers now, and will contribute back my findings. Should I contribute my findings to this PR, or open a fresh one? |
|
Sounds promising, @aguibert. Could you perhaps compare the start up times between the two containers? Is the long start up time a regression? Did you start it with the default configuration which creates a database on startup? That could perhaps explain (some) of the differences. In our custom Db2Container I've set up DB2 to run in some sort of a (unsupported) in-memory setup. In our use case we're destroying the container anyway and therefore don't care about the durability of any data. Extra configuration: Setting up the tmpfs: Creating the database: |
|
@frederikb thanks for the suggestions. I didn't do a detailed comparison, but here is a rough breakdown of the start time activities on my machine:
I'll inquire on the "configuring instances" second pass, as well as the text search service, which perhaps can be disabled with an extra switch. However, this biggest time sink was creating the DB itself (50s) and I doubt that can be sped up further, but I will check. |
|
I put an update to the container here - https://hub.docker.com/r/ibmcom/db2 to address some of the concerns with the start up time. There is more to be done, so subsequently we will continue making updates. |
|
Thanks, @irinadel! I'm still seeing something in the if [[ $1 = "db2start" ]]; then
su - ${instance_name?} -c "db2start"
fiI'm pretty sure that Workaround: Set the environment variable |
|
@frederikb that piece of code path you showed never executes, so never mind it, its legacy, ill remove it on the next iteration
|
|
@irinadel you're absolutely right. That was a leftover from our previous integration of the db2express-c image. |
|
@kiview @bsideup I opened a new PR (#1611) for the a DB2 module with the new DB2 image. Can one of you please review it? Sorry for the separate PR, but when I tried to do a PR from |
Created during the Softwerkskammer Ruhr Hackergarten together with Felix Winkler.
I have some concerns regarding the license issues, similar to Oracle. On the one hand, we to provide an environment variable to accept the license when running the official IBM DB2 image.
The other problem is the JDBC driver. IBM doesn't provide the JAR on Maven Central. A search on Maven Central resulted in finding this Maven Repository:
https://artifacts.alfresco.com/nexus/content/repositories/public/Any good ideas how to proceed from here?
The general implementation works, as well as the JDBC-URL support, however the JDBC-URL support has some vendor specific SQL, so the test fails.
closes #432