Code review comment for lp://staging/~ev/daisy/weighted-machines

On Mon, Mar 18, 2013 at 05:47:22PM -0000, Evan Dandrea wrote:
> Evan Dandrea has proposed merging lp:~ev/daisy/weighted-machines into lp:daisy.
>
> Requested reviews:
> Daisy Pluckers (daisy-pluckers)
>
> For more details, see:
> https://code.launchpad.net/~ev/daisy/weighted-machines/+merge/153885
>
> This adds support to daisy for the new update_errors_by_release method in oops-repository. It also provides a idempotent script for populating the existing data into the new FirstError and ErrorsByRelease column families. This is for bug 1077122.
> --
> https://code.launchpad.net/~ev/daisy/weighted-machines/+merge/153885
> You are subscribed to branch lp:daisy.

> === modified file 'daisy/utils.py'
> --- daisy/utils.py 2013-03-09 01:19:57 +0000
> +++ daisy/utils.py 2013-03-18 17:46:20 +0000
> @@ -1,6 +1,7 @@
> from oopsrepository import oopses
> import apt
> import os
> +import uuid
>
> def get_fields_for_bucket_counters(problem_type, release, package, version):
> fields = []
> @@ -78,6 +79,11 @@
> if version:
> oopses.update_bucket_versions(oops_config, crash_signature, version)
>
> + if hasattr(oopses, 'update_errors_by_release'):
> + if (system_uuid and release) and not third_party:

Why are you excluding third_party here? If we are recording errors for
systems with third party packages shouldn't they factor into the
calculation too?

> + oops_uuid = uuid.UUID(oops_id)
> + oopses.update_errors_by_release(oops_uuid, system_uuid, release)
> +

In the build_errors_by_release tool you check to ensure that release
starts with Ubuntu. Is the same check missing above?

> def attach_error_report(report, context):
> # We only attach error report that was submitted by the client if we've hit
> # a MaximumRetryException from Cassandra.

The rest of the merge proposal looks good to me.

--
Brian Murray