Code review comment for lp:~egoist-dev/charms/precise/mongodb/replica-set

Thanks for this work. It looks like the mongo charm certainly needs a polish.

This branch contains conflict markers, so certainly needs repair as the Python will not compile.

Under what circumstances is it sane to change replicaset_server_dbpath or replicaset_server_logpath? If there are no genuine reasons, the charm should be opinionated and the paths hardcoded, rather than having them as configuration items (which really requires tests to confirm things work with non-default paths).

Is there a reason pymongo needs to be installed via pip, or can we use the package? Many installations will not have network connectivity to pypi, so a package is preferred. Otherwise, at a minimum a configurable URL will need to be added to config.yaml, specifying where the tarball can be downloaded and installed from.

There are some mixed tabs and spaces in hooks.py that should be fixed. Quite a lot of the changes appear to be whitespace, replacing spaces with tabs, so I suspect editor settings have caused problems. For example:

- s.shutdown(socket.SHUT_RDWR)
- juju_log("port_check: %s:%s/%s is open" % (host, port, protocol))
- return(True)
+ s.shutdown(socket.SHUT_RDWR)
+ juju_log("port_check: %s:%s/%s is open" % (host, port, protocol))
+ return(True)

At a minimum, it needs to be consistent. I'm seeing all three acceptable forms of indentation being used in the one file, which is a timebomb (4 space only, mixed 4 space + tab, tab only).

A lot of the helpers here could instead be pulled from charm-helpers, in particular relation_get and relation_set. I suspect they would disappear from this review if the whitespace returned to normal.

This code is broken. relation-get will return the same information consistently throughout the hook (and if it doesn't, you have found a Juju bug).

+ while relation_data is None and current_try < max_tries:
+ time.sleep(wait_for)
+ relation_data = json.loads(subprocess.check_output(relation_cmd_line))

I worry about there being race conditions adding and removing the database from the replica set. It seems that the current replica set configuration is being update, and the entire modified configuration resubmitted. If multiple peer relation-changed or relation-departed hooks are running simultaneously, they will race and we will end up with broken replication configuration. I don't think this can be fixed without juju leadership.