You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/04/18 07:31:56 UTC
[GitHub] [druid] FrankChen021 opened a new pull request #11127: Set default value of ZK_VERSION in docker compose file
FrankChen021 opened a new pull request #11127:
URL: https://github.com/apache/druid/pull/11127
### 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:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (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](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #11127:
URL: https://github.com/apache/druid/pull/11127#issuecomment-823710760
Thanks @xvrl , I close this PR and wait for your PR to be merged.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11127:
URL: https://github.com/apache/druid/pull/11127#discussion_r615641001
##########
File path: integration-tests/pom.xml
##########
@@ -435,7 +435,6 @@
<docker.build.skip>false</docker.build.skip>
<docker.build.hadoop>false</docker.build.hadoop>
<it.indexer>middleManager</it.indexer>
- <zk.version>3.5</zk.version>
Review comment:
> Will Any change to .travis.yml be reflected on the CI in this PR ?
Yes. It will.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #11127:
URL: https://github.com/apache/druid/pull/11127#discussion_r615626344
##########
File path: integration-tests/pom.xml
##########
@@ -435,7 +435,6 @@
<docker.build.skip>false</docker.build.skip>
<docker.build.hadoop>false</docker.build.hadoop>
<it.indexer>middleManager</it.indexer>
- <zk.version>3.5</zk.version>
Review comment:
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.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11127:
URL: https://github.com/apache/druid/pull/11127#discussion_r615603773
##########
File path: integration-tests/pom.xml
##########
@@ -435,7 +435,6 @@
<docker.build.skip>false</docker.build.skip>
<docker.build.hadoop>false</docker.build.hadoop>
<it.indexer>middleManager</it.indexer>
- <zk.version>3.5</zk.version>
Review comment:
it should be removed from .travis.yml 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
xvrl commented on pull request #11127:
URL: https://github.com/apache/druid/pull/11127#issuecomment-823628859
@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.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 closed pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
FrankChen021 closed pull request #11127:
URL: https://github.com/apache/druid/pull/11127
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
xvrl commented on pull request #11127:
URL: https://github.com/apache/druid/pull/11127#issuecomment-822680658
thanks @FrankChen021 I'm actually planning to remove the need for this environment variable in https://github.com/apache/druid/pull/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.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #11127: Set default value of ZK_VERSION in docker compose file
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #11127:
URL: https://github.com/apache/druid/pull/11127#issuecomment-822916944
> 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 ?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org