Skip to content

CouchDB RA - added replication more then one database, please review RA. #23

Closed
wants to merge 8 commits into from

4 participants

@fghaas
Cluster Labs member
fghaas commented on e817e5a Jun 16, 2011

Before getting into this: this agent should live in heartbeat/. Don't invent your own testing directory.

@fghaas
Cluster Labs member

This is no longer in line with current upstream. Check how the other RAs in heartbeat/ are doing this.

rnz replied Jun 16, 2011

I know, but in Ubuntu Server 11.04 use old line, and i use testing dir for my convenience... It fix later...

Cluster Labs member

To make this clear from the start: since you are submitting this for review for upstream inclusion, then this has to be fixed before the review can continue. If you can't spare time for this now, then let us know, but the review will not proceed until you can.

@fghaas
Cluster Labs member

how about [ -z "$OCF_RESKEY_username" ] ?

@fghaas
Cluster Labs member

This log message is likely to be confusing.

@fghaas
Cluster Labs member

The binary is named couchdb, the RA name is couchdbctrl. They should agree.

@fghaas
Cluster Labs member

Use HA_RSCTMP, not HA_VARRUN.

@fghaas
Cluster Labs member

Duplicate parameter.

@fghaas
Cluster Labs member

Should be boolean.

rnz replied Jun 16, 2011

I did not think to lay out the parameters of replication, because they are transmitted CURL as JSON argument to the server:
{"source":"http://admin:pass@remoteserver:port/dbname", "target":"http://admin:pass@127.0.0.1:port/dbname", "continuous":true}

@fghaas
Cluster Labs member

Should be integer.

@fghaas
Cluster Labs member

Following precedent set by other RAs, it's OK to name this just "pid".

@fghaas
Cluster Labs member

Bad idea. What you want to do instead is set timeouts on the stop operations, which the RA can then access via the OCF_RESKEY_CRM_meta_timeout envar during stop.

@fghaas
Cluster Labs member

You probably want to make cmd a local variable.

@fghaas
Cluster Labs member

Kill this if block. Use ocf_run instead.

@fghaas
Cluster Labs member

Pointless. Use ocf_run below instead and you'll get all the logging you need.

@fghaas
Cluster Labs member

Again, use ocf_run and you'll avoid this useless log message.

@fghaas
Cluster Labs member

Pointless convenience function. Just open-code exit $OCF_ERR_<errcode> when you need it.

@fghaas
Cluster Labs member

Why are you jumping through hoops here? If you can't figure out the correct pid, it's better to fail rather than guessing what the correct pid is and potentially killing the wrong instance.

@fghaas
Cluster Labs member

Commented-out code blocks make a review needlessly tedious. Just remove those when you submit for review.

@fghaas
Cluster Labs member

Again, ocf_run.

@fghaas
Cluster Labs member

This stopgracetime thing is, as pointed out above, useless. Just loop on monitor after stop, and let the cluster manager time you out. Check OCF RA dev guide for examples.

rnz replied Jun 16, 2011

"Check OCF RA dev guide for examples" - where is it? https://github.com/ClusterLabs/resource-agents/blob/master/heartbeat/Dummy ?

@fghaas
Cluster Labs member

suggest to use "ocf_run rm -vf" here

@fghaas
Cluster Labs member

Here too.

@fghaas
Cluster Labs member

Again, commented-out code block. Either fix this, or kill it.

@fghaas
Cluster Labs member

Use check_binary.

@fghaas
Cluster Labs member

Can you please rephrase your log messages so they're helpful for people other than you?

@fghaas
Cluster Labs member

What? Explain please.

@fghaas
Cluster Labs member

That's assuming there will ever only be one instance of one couchdb resource running on any node. I doubt that that's a valid assumption.

rnz replied Jun 16, 2011

if respawntimeout > 0 : couchdb may autorestart and pid changing...

Cluster Labs member

It is not a good idea to have anything outside the cluster manager restart the resource. Make sure couchdb does not automatically restart. It's Pacemaker's job to detect resource failure and recover from it.

rnz replied Jun 16, 2011

ok: respawntimeout = 0 always

@fghaas
Cluster Labs member

As noted above, I think this should be a boolean, and if so it should be tested with ocf_is_true.

@fghaas
Cluster Labs member

That's definitely an OCF_ERR_PERM.

@fghaas
Cluster Labs member

ocf-shellfuncs already sets this default.

@fghaas
Cluster Labs member

No no no no no! You're not reversing a built in default for a meta variable.

@fghaas
Cluster Labs member

What are you doing here? Have you understood the concept of anonymous and non-anonymous clones?

@fghaas
Cluster Labs member

HA_RSCTMP again, and what if the user chooses to override the default?

@dmuhamedagic

Nothing moving here for quite a while. Where did you get stuck?

@fghaas fghaas was assigned Oct 30, 2012
@dmuhamedagic

Still stuck? Can somebody update status? @fghaas, can this RA be included?

@fghaas
Cluster Labs member
fghaas commented Oct 30, 2012

Not a clue. Specifically since 53f3ee9 seems to be a near-complete rewrite.

@dmuhamedagic

@rnz any news here?

@dmuhamedagic

Seems like nobody cares about this one anymore. Will close next week if nobody comments.

@krig
Cluster Labs member
krig commented Feb 22, 2016

Since there hasn't been any update since November, I am closing this.

@krig krig closed this Feb 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.