Code review comment for lp:~adeuring/charmworld/queue-review-mutex

On 27.02.2013 18:30, <email address hidden> wrote:
> lgtm with a check on using safe vs fsync. If safe works then it should
> be a bit more performance than an fsync.

Right, using safe=True works also. Checking if this parameter has been
specififed, is a bit more tricky, so I changed the function lock() to
just try to insert a test record twice.

>
> I also would prefer to see something that catches locks that were not
> released. I know they timeout, but we should have some notice that they
> were hit so we can look into the issue. I'm worried about a charm
> instance dying without us realizing there's an issue. It might take some
> coordination with the charm though to get an error/notice out and up to
> nagios or email or something.

If a job "just dies" without releasing the job, we have a problem that
is likely much more serious that just a stale lock:

    try:
        yield
    finally:
        locks.remove(my_lock)

ensures that the lock is released for "regular Python errors". If the
job dies due to a segfault, this segfault would be my main concern, not
the "orphaned" lock data.

>
>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/core_review.py
>
> File charmworld/jobs/core_review.py (right):
>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/core_review.py#newcode107
>
> charmworld/jobs/core_review.py:107: c = pymongo.Connection(MONGO_URL,
> fsync=True)
> can we get away with just safe=True vs fsync? It's a bit lighter than
> fsync and we changed to this in test runs to help fix issues from mongo
> syncing.

Done. Remember though that we can use fsync=False -- the odd thing is
that specifying fsync=True as well as fsync=False ensure that inserting
a doc with an existing key raises a DuplicateKeyError, while _not_
specifying it suppresses the DuplicateKeyError.

>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/lp.py
> File charmworld/jobs/lp.py (right):
>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/lp.py#newcode73
>
> charmworld/jobs/lp.py:73: connection = getconnection(settings)
> does this need to have the safe= flag set to get a safe connection?

No, the trick here are the default parameters of getconnection():

def getconnection(settings, fsync=False, safe=False):
    [...]
    connection = pymongo.Connection(
        url_or_host, port,
        fsync=fsync,
        safe=safe,
    )

So, fsync _is_ specified and everyting is fine.

>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/review.py
> File charmworld/jobs/review.py (right):
>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/review.py#newcode117
>
> charmworld/jobs/review.py:117: c = pymongo.Connection(MONGO_URL,
> fsync=True)
> again, can safe= work?

changed.

>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/tests/test_utils.py
>
> File charmworld/jobs/tests/test_utils.py (right):
>
> https://codereview.appspot.com/7379053/diff/1/charmworld/jobs/tests/test_utils.py#newcode38
>
> charmworld/jobs/tests/test_utils.py:38: self.assertIs(None,
> self.db['locks'].find_one({'_id': LOCK_NAME}))
> should this be wrapped into a helper? utils.check_lock(db, LOCK_NAME) or
> something?

A matter of taste, I think ;) Personally, Id don't see much benefit in
changing a one-line call with another one-line call. And I prefer to see
the details where they are needed, in this case, that find_one() looks
for any record with the right ID.

>
> https://codereview.appspot.com/7379053/diff/1/default.ini
> File default.ini (right):
>
> https://codereview.appspot.com/7379053/diff/1/default.ini#newcode42
> default.ini:42: script_lease_time = 30
> we need the bug to note that the charm should be updated to create this
> time based on the config time used to cron the importer.
>
> https://codereview.appspot.com/7379053/
>