-
Notifications
You must be signed in to change notification settings - Fork 442
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
Update async replication example and add semisync replication example #547
Conversation
Pull a 11.0.4 version should fix the "Access denied for user" from healthcheck, it make a new user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very glad to see the use of yml based syntax to improve the readabilty, remove duplication etc.
Just some small fixes to keep examples as minimal as possible. And a few questions about why things where included.
examples/.env
Outdated
@@ -0,0 +1,7 @@ | |||
MARIADB_ROOT_PASSWORD=secret | |||
MYSQL_INITDB_SKIP_TZINFO=Y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is MARIADB_INITDB_SKIP_TZINFO, but is there really a need to exclude tz init? Generally like to keep examples small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack will remove.
MARIADB_REPLICATION_PASSWORD=replicationpass | ||
PRIMARY_name='mariadb-primary' | ||
REPLICATION_name_1='mariadb-replica-1' | ||
REPLICATION_name_2='mariadb-replica-2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REPLICATION_*
are only used once. Can we just omit them and get the default configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are here giving the exact name of replicas, I personally prefer to specify the name of replicas.
x-common-attributes: &common-attributes | ||
image: mariadb:lts | ||
env_file: | ||
- .env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in preparation for non-replication based examples here, recommend explicit naming about env_replication
as a env_file
.
What's the gain on having it as an env file? It it just showcasing the hiding of passwords out of the main compose file as best practices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gain as you said for password and having it in place and not repeating the variables for different containers.
Note this is docker-compose
file and it has to be used env_file
with specifiyng .env
.
test: ["CMD", "healthcheck.sh", "--connect", "--replication_io", "--replication_sql", "--replication_seconds_behind_master=1", "--replication"] | ||
interval: 10s | ||
timeout: 5s | ||
retries: 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is start_period worth including and works correctly?10s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it works, have added, thanks. There is also start_interval, but I wouldn't added it here.
- *common-replication | ||
container_name: ${REPLICATION_name_1} | ||
command: --server-id=2 --log-basename=mariadb --rpl_semi_sync_slave_enabled | ||
environment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can environment
and depends_on
go into common-replication
? (in the hope that environment
is additive)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can not move environment
but we can do depends_on
and that I have added. Only thing that could work is that we specify MARIADB_MASTER_HOST=${PRIMARY_name}
and MARIADB_HEALTHCHECK_GRANTS=REPLICA MONITOR
in .env
but I think this way is more readable.
examples/compose-replication.yml
Outdated
interval: 10s | ||
timeout: 5s | ||
retries: 3 | ||
logging: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is logging
needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As default login setting for all services set explicitly. I'm ok if you want to exclude it.
examples/compose-replication.yml
Outdated
retries: 3 | ||
logging: | ||
driver: journald | ||
networks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a dedicated network created for the compose file? Is there something gained by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but with auto-generated name, I don't prefer this. It is gained when we create the new front-end service, or new service for backend, intuitively we know what is missing and what to add and where.
5abef0b
to
5294858
Compare
Overview
The PR consists of 2 commits :
Update of docker compose example for async replication
When run
compose-replication.yml
docker-compose file, it doesn't start healthy service for primaryPrimary service health failure
Add semi-sync example