Code review comment for lp:~evarlast/charms/trusty/mongodb/fix-dump-actions

Revision history for this message
Matt Bruzek (mbruzek) wrote :

Hello Jay,

While reviewing the queue today and noticed more than one mongodb charm merge proposals. Since we review the oldest first this proposal did not get reviewed first.

As Ryan points out in a bug 1518468 ( https://bugs.launchpad.net/charms/+source/mongodb/+bug/1518468 ) using sleep funtions are not a deterministic way to build tests that run on multiple environments as the times can be very long on some (azure) and lower on others (aws). This merge proposal changes some sleep() calls to sentry.wait() but introduces several more time.sleep() commands and the latter seems to be the wrong way to go.

## 03_deploy_replicaset.py
The new code contains the comment “# We assume minimum unit number is master.” There should be a better way to get the master address in the test. Again you may be able to run commands on the units to determine their master relationship, or use a mongo python library to determine the master reliably.

The tests passed for me on AWS and I am going to merge this code. I would encourage you to change the test code to detect the state of the mongodb deployment from reliable sources that do not involve sleeping for arbitrary amounts of time. Such as running a command on the unit to tell when it is ready, or using `juju status-history unit/0` to determine ready states.

review: Approve

« Back to merge proposal