On 02/22/2010 04:14 AM, Marian Marinov wrote: > Florian, > I made significant changes to the mysql RA but I feel that it will be very > hard > to make it work only with promote/demote functions. > > [...] > > I'm stuck with this. My RA which I use in production is almost entirely > implemented in the notify function but I really think that this is not the > way > a public RA should be implemented. > > So if you have any suggestions I'll be happy to hear them.
Let's get a few issues out of the way first, so we can then tackle the
actual master/slave capability.
> --- a/heartbeat/mysql Tue Feb 09 13:07:37 2010 +0100
> +++ b/heartbeat/mysql Mon Feb 22 19:46:51 2010 +0100
> @@ -1,24 +1,21 @@
> -#!/bin/sh
> +#!/bin/bash
While I'm not the type to bust out in "we must be portable" chants and
demand that no bashisms be ever used, you used some fairly intricate
bash features in the RA. Have you double-checked that this stuff works
as expected on "legacy" platforms such as RHEL/CentOS 5?
> -# OCF_RESKEY_test_passwd
> +# OCF_RESKEY_test_password
Please don't. You're breaking existing installations. You may think that
"passwd" is stupid and "password" is much better, and I'd agree, but
compatibility wins.
> +# OCF_RESKEY_replication_aware
> +# OCF_RESKEY_replication_user
> +# OCF_RESKEY_replication_pass
I'd say OCF_RESKEY_replication_aware is superfluous. Check for the
OCF_RESKEY_CRM_meta_master_max envar instead. If it's present and
greater than zero, then the resource is configured as a master/slave,
which automatically means it should be "replication aware".
Plus, just so users don't get needlessly confused, make that
"replication_passwd" please.
> -: ${OCF_FUNCTIONS_DIR=${OCF_ROOT}/resource.d/heartbeat}
> -. ${OCF_FUNCTIONS_DIR}/.ocf-shellfuncs
> +. ${OCF_ROOT}/resource.d/heartbeat/.ocf-shellfuncs
Please rebase your RA on the current standard, which is the one with the
configurable $OCF_FUNCTIONS_DIR.
> +: ${OCF_RESKEY_replication_aware_default=${OCF_RESKEY_replication_aware}}
> +: ${OCF_RESKEY_replication_user_default=${OCF_RESKEY_replication_user}}
> +: ${OCF_RESKEY_replication_pass_default=${OCF_RESKEY_replication_pass}}
Whoa, you got those exactly backwards. :)
> - usage: $0 (start|stop|validate-all|meta-data|monitor)
> + usage: $0 (start|stop|validate-all|meta-data|monitor|notify)
Missing promote|demote.
> -<shortdesc lang="en">Manages a MySQL database instance</shortdesc>
> +<shortdesc lang="en">MySQL resource agent</shortdesc>
Again, please rebase your patch to the current upstream.
> -<action name="monitor" depth="0" timeout="30" interval="10" />
> +<action name="monitor" depth="0" timeout="20" interval="20" start-delay="0"
> role="Slave" />
> +<action name="monitor" depth="0" timeout="20" interval="10" start-delay="0"
> role="Master" />
Adding monitor ops for Master and Slave is fine, but don't remove the
one bound to no role.
> - ocf_log err "mysqld binary $OCF_RESKEY_binary does not exist or is not
> executable";
> - return $OCF_ERR_INSTALLED;
> + ocf_log err "mysqld binary $OCF_RESKEY_binary does not exist or
> is not executable";
> + return $OCF_ERR_INSTALLED;
The patch seems to have quite a few of these whitespace-only changes.
Can you fix those so we see only the functional changes?
> My latest version can be found here:
> http://hydra.azilian.net/gitweb/?p=linux-ha/.git;a=summary
Upstream uses mercurial. Any particular reason for keeping your stuff in
a git repo instead? It would be tremendously helpful if we could get
your patch, in "hg export" format, with the above cleanups. That way we
would have everything non-essential out of the way and we could focus on
the actual code review. Thanks!
Cheers,
Florian
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Linux-HA mailing list [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha See also: http://linux-ha.org/ReportingProblems
