Set default value of ZK_VERSION in docker compose file

[FrankChen021]

Description

#10786 Introduced an environment variable ZK_VERSION in docker-compose.base.yml and this env variable is set to 3.5 in pom.xml of integration-test module and the travis file .travis.yml .

In the README of integration-test module, there's a section describing how to manually bring up Druid cluster by docker-compose. And because of lacking of description that ZK_VERSION must be set, following the steps in that section leads to failure of starting up druid-zookeeper-kafka service.

Of course one way to solve that problem is by updating the README to add a step to tell users to set ZK_VERSION before manually bringing up Druid cluster. But I think it's not friendly for users to do this because ZK_VERSION is an interval environment variable mainly for backward compatibility and not a variable exposed to users.

So, I make a slight change to the compose file to set the default value of ZK_VERSION to 3.5, so that users don't care about setting this value.

• If ZK_VERSION is not set by shell environment variable, it will default to 3.5
• If ZK_VERSION is set by shell environment variable, it will use the value set by shell

Such behaviors are compatible with current ways.

Since the default value of ZK_VERSION is set in compose file, I also remove zk.version property from pom.xml in integration-test module to simply the management of default version.

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[abhishekagarwal87]

it should be removed from .travis.yml too?

[FrankChen021]

Good question. I think it should be, but I'm unable to validate this change on my local so I leave it there. Will Any change to .travis.yml be reflected on the CI in this PR ? If it's, I can push a commit to delete the default version in that file.

[abhishekagarwal87]

Will Any change to .travis.yml be reflected on the CI in this PR ?

Yes. It will.

[xvrl]

thanks @FrankChen021 I'm actually planning to remove the need for this environment variable in #11073. Since we want to keep the maven ZK version in sync with what we run integration tests against, we may need to adjust things a little.

[FrankChen021]

thanks @FrankChen021 I'm actually planning to remove the need for this environment variable in #11073. Since we want to keep the maven ZK version in sync with what we run integration tests against, we may need to adjust things a little.

Thanks for you information. I checked the PR you mentioned, and found that there's no change made to integration-tests/docker/service-supervisords/zookeeper.conf which references ZK_VERSION environment variable. That means setting of this env variable is still needed before manually executing 'docker-compose' command to bring up Druid cluster. The core change in this PR is setting this environment variable in docker file instead of pom.xml in integration-test module. Do you plan to make an adjust there ?

[xvrl]

@FrankChen021 I see your point, I updated #11073 to completely remove the need to specify ZK_VERSION at runtime when spinning up docker-compose. The variable is still required at build time to build the docker image, but that's no different than other docker build arguments such as MYSQL_VERSION that we already have to specify. Hope this satisfies your concerns.

[FrankChen021]

Thanks @xvrl , I close this PR and wait for your PR to be merged.