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

Add compose file for multi-cluster setup #4032

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Mar 3, 2021

What changed?
Add compose file for multi-cluster setup

Why?
It makes it easier for us to run cadence with 2 clusters on laptop.

How did you test it?

Potential risks

@Shaddoll Shaddoll requested review from a team and yux0 March 3, 2021 17:49
environment:
- "CASSANDRA_SEEDS=cassandra"
- "STATSD_ENDPOINT=statsd:8125"
- "DYNAMIC_CONFIG_FILE_PATH=config/dynamicconfig/development.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

The replication has to be config from the yaml file. Take a look at the development_active.yaml and development_standby.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it uses config_template.yaml to generate config file. This environment variable is set to get dynamic config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. This is a dynamic config yaml. Then it looks good to me.

docker/config_template.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

Please do not use active/standby to name clusters. This will confuse users.

@coveralls
Copy link

coveralls commented Mar 3, 2021

Coverage Status

Coverage decreased (-0.09%) to 64.498% when pulling 05d35be on Shaddoll:multi-docker into a278995 on uber:master.

Comment on lines +38 to +41
- "7943:7933"
- "7944:7934"
- "7945:7935"
- "7949:7939"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we make the port number consistent with those in development_standby.yaml? Makes it a bit easier when switching back and forth between different setup.

  bootstrapHosts: [ "127.0.0.1:8933", "127.0.0.1:8934", "127.0.0.1:8935", "127.0.0.1:8940" ]

Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

Nit: could you also make change to config/ that:

development_active.yaml -> development_primary.yaml
development_standby.yaml -> development_secondary.yaml

And change the related README.md
Or create a ticket and do it separately :D

@Shaddoll Shaddoll merged commit aae8fb9 into cadence-workflow:master Mar 4, 2021
yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021
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.

5 participants