Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(docker): refactor docker build scripts #1687

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

mars-lan
Copy link
Contributor

@mars-lan mars-lan commented Jun 2, 2020

  • add "build" option to docker-compose files to simplify rebuilding of images
  • create "start.sh" script so it's easier to override "command" in the quickstart's docker-compose file
  • use dockerize to wait for requisite services to start up
  • add a dedicated Dockerfile for kafka-setup

This fixes #1549 & #1550

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

- add "build" option to docker-compose files to simplify rebuilding of images
- create "start.sh" script so it's easier to override "command" in the quickstart's docker-compose file
- use dockerize to wait for requisite services to start up
- add a dedicated Dockerfile for kafka-setup

This fixes datahub-project#1549 & datahub-project#1550
```
This command will start the container. If you have the image available in your local store, this image will be used
for the container otherwise it will download the `latest` image from Docker Hub and then start that.
To start a container using an existing image, run the same command without the `--build` flag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be good to note that: Once you build an image in your local, docker-compose up will simply use already built image, it will not pull the latest image from Docker Hub. You need to force it to pull the latest image by running docker-compose pull before running up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the intention of these Dockerfiles is for local development, instead of quickstart. As such pulling the latest image from docker hub is probably not a common operation in this case?

for the container otherwise it will download the `latest` image from Docker Hub and then start that.
This command will rebuild the local docker image and start a container based on the image.

To start a container using an existing image, run the same command without the `--build` flag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

```
This command will start the container. If you have the image available in your local store, this image will be used
for the container otherwise it will download the `latest` image from Docker Hub and then start that.
To start a container using a previously built image, run the same command without the `--build` flag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

```
This command will start the container. If you have the image available in your local store, this image will be used
for the container otherwise it will download the `latest` image from Docker Hub and then start that.
To start a container using a previously built image, run the same command without the `--build` flag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Comment on lines -97 to -98
KAFKA_BROKER_ID: ignored
KAFKA_ZOOKEEPER_CONNECT: ignored
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add this env vars here now that Dockerfile use those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment on lines 194 to 195
command: "sh -c 'while ping -c1 kafka-setup &>/dev/null; do echo waiting for kafka-setup... && sleep 1; done; \
echo kafka-setup done! && /start.sh'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, GMS doesn't strictly need to wait for kafka-setup unlike mxe consumers. So, this can simply be a './start.sh'. But, anyways, doesn't hurt to wait for that either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GMS also sends MAE right? Wouldn't it need to wait for the topic creation as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it doesn't because it's the producer. Even if topic is not created, when GMS starts to produce, topic will be automatically created because auto.create.topics.enable is true by default. http://kafka.apache.org/documentation.html#auto.create.topics.enable

However, topics are not created on Kafka, mxe consumers fails during initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Yeah auto topic creation doesn't work for Kafka stream apparently: https://docs.confluent.io/current/streams/developer-guide/manage-topics.html

@jywadhwani jywadhwani requested a review from keremsahin1 June 8, 2020 17:53
@mars-lan mars-lan merged commit 4f221f9 into datahub-project:master Jun 8, 2020
mars-lan added a commit to mars-lan/datahub that referenced this pull request Jun 25, 2020
* build(docker): refactor docker build scripts

- add "build" option to docker-compose files to simplify rebuilding of images
- create "start.sh" script so it's easier to override "command" in the quickstart's docker-compose file
- use dockerize to wait for requisite services to start up
- add a dedicated Dockerfile for kafka-setup

This fixes datahub-project#1549 & datahub-project#1550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make mce-consumer-job container wait for GMS
2 participants