Florian, I greatly appreciate the comments. About the bash/sh. I prefer bash since I can use some neat things like REGEX directly. I'm developing on CentOS 5.3 and testing also on CentOS 5.2. I don't have CentOS 5 installed currently but by the end of the week I will have :)
If it is your or project's desire to use /bin/sh. I don't mind I can do it.
replication_aware - this was a first think I think of, thanks for the
suggestion already implemented
passwd/password - fixed
whitespaces - sorry but the script was not very well indented and I need it
indented in order to work. I'll try to clean this up for you. I understand
what is the problem you are facing.
I keep my stuff in git, currently preparing all of my project to be published
with it. I have a mercurial clone of the agents. And I'll switch to it for the
linux-ha projects, soon (hopefully by the middle of next week).
I have updated my git version of the mysql RA.
I have also updated the mercurial repo but I still have whitespaces there.
I'll clean those up and publish the whole mercurial repo.
Marian
On Monday 22 February 2010 21:47:36 Florian Haas wrote:
> 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
>
--
Best regards,
Marian Marinov
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Linux-HA mailing list Linux-HA@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha See also: http://linux-ha.org/ReportingProblems
