1237491 - Use new BMO MozReview batch-attachment API

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[Mark Côté [:mcote]]

As a prerequisite for bug 1211791 (and since MozReview doesn't have partial-series landings yet), we need to group all the calls to Bugzilla for updating and creating new attachments.  This will make it simple to move to a single call when the new Bugzilla API is done (bug 1226028).

Comment 1

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Step one in consolidating BMO attachment calls on review publishing is to move
them all to a single function in rbbz/extension.py.

Review commit: https://reviewboard.mozilla.org/r/29809/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29809/

Comment 2

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

This is another step toward switching to a single Bugzilla API call for
attachment actions.  We now prep all the attachment adds and updates and
then execute them sequentially.  This also gives a slight perf boost for
large commit sets since we only grab a bug's attachments once per set,
instead of once per commit.

Review commit: https://reviewboard.mozilla.org/r/29811/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29811/

Comment 3

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29811/#review26635

::: pylib/mozreview/mozreview/bugzilla/client.py:50
(Diff revision 1)
> +

Right, I need a docstring here.

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/29809/#review26647

::: pylib/rbbz/rbbz/extension.py:242
(Diff revision 1)
> +            email = user_email_cache.get(bum.bugzilla_user_id)
>  
> +            if email is None:
> -        user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
> +                user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
>  
> -        # Since we're making the API call, we might as well ensure the
> +                # Since we're making the API call, we might as well ensure the
> -        # local database is up to date.
> +                # local database is up to date.
> -        users = get_or_create_bugzilla_users(user_data)
> +                users = get_or_create_bugzilla_users(user_data)
> -        reviewers[users[0].email] = False
> +                email = users[0].email
> +                user_email_cache[bum.bugzilla_user_id] = email

The caching is unrelated to this refactor. Next time, please break it out into a separate commit.

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/29809/#review27053

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/29811/#review27057

::: pylib/mozreview/mozreview/bugzilla/client.py:173
(Diff revision 1)
> +    def do_updates(self):

Do we need to change error handling here?

I'm concerned that one error will cause us to drop any pending actions that may have succeeded when we were performing them one at a time.

::: pylib/mozreview/mozreview/bugzilla/client.py:182
(Diff revision 1)
> +        self.creates = []

I assume this is safe because of the object lifetime, but if we hit an exception in the code above and then do_updates() was called again, would it be possible that we would attempt to repeat some of these actions even if they had already been performed?

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/1-2/

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/1-2/

Comment 9

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29811/#review27057

> Do we need to change error handling here?
> 
> I'm concerned that one error will cause us to drop any pending actions that may have succeeded when we were performing them one at a time.

I'm  passing this off to mdoglio, so I'll just drop these issues.  He'll follow up and see if they're something to be concerned about.

Comment 10

[Mauro Doglio [:mdoglio]]

:mcote last thing we said is that you are finishing this, right?

Comment 11

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29811/#review27057

> I'm  passing this off to mdoglio, so I'll just drop these issues.  He'll follow up and see if they're something to be concerned about.

Taking this back.  So I'm not sure what you mean... we currently go through the children, obsoleting attachments if necessary, then posting or updating attachments if needed.  We publish them after.  My changes work similarly, but now wet get all the information required first, the iterate through the changes.  In my next commit, even that iteration will be replaced by a single BMO API call.

So I think we're good here, but please correct me if I've misunderstood your concern.

Comment 12

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29811/#review27057

> I assume this is safe because of the object lifetime, but if we hit an exception in the code above and then do_updates() was called again, would it be possible that we would attempt to repeat some of these actions even if they had already been performed?

Yeah not in how it's used right now, but it is a potential concern.  I'm going to be replacing it with a single API call, but for now I've changed it to remove entries as they are processed.

Comment 13

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/2-3/

Comment 14

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/2-3/

Comment 15

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/3-4/

Comment 16

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/3-4/

Comment 17

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/4-5/

Comment 18

[Mark Côté [:mcote]]

Attachment: MozReview Request: testing: Set mozreview_app_id in BMO params (bug 1237491). r?smacleod

Review commit: https://reviewboard.mozilla.org/r/39255/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39255/

Comment 19

[Mark Côté [:mcote]]

Attachment: MozReview Request: testing: Optionally set BMO API key description (bug 1237491). r?smacleod

Review commit: https://reviewboard.mozilla.org/r/39257/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39257/

Comment 20

[Mark Côté [:mcote]]

Since the new API has landed and been fixed, I'm going to include the actual use of it in this commit set (new commit forthcoming as soon as I finish another test run).

Comment 21

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/4-5/

Comment 22

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/5-6/

Comment 23

[Mark Côté [:mcote]]

Comment on attachment 8729128 [details]
MozReview Request: testing: Set mozreview_app_id in BMO params (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39255/diff/1-2/

Comment 24

[Mark Côté [:mcote]]

Comment on attachment 8729129 [details]
MozReview Request: testing: Optionally set BMO API key description (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39257/diff/1-2/

Comment 25

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod

Review commit: https://reviewboard.mozilla.org/r/39797/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39797/

Comment 26

[Steven MacLeod [:smacleod]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/29809/#review36745

::: pylib/mozreview/mozreview/bugzilla/attachments.py:66
(Diff revision 5)
> -    for review in relevant_reviews:
> +        for review in relevant_reviews:
> -        if review.user == last_user:
> +            if review.user == last_user:
> -            # We only care about the most recent review for each
> +                # We only care about the most recent review for each
> -            # particular user.
> +                # particular user.
> -            continue
> +                continue
>  
> -        last_user = review.user
> +            last_user = review.user

please take this chance to switch this to `mozreview.review_helpers.gen_latest_reviews`

::: pylib/mozreview/mozreview/bugzilla/attachments.py:92
(Diff revision 5)
> -    if review_request_draft.get_latest_diffset():
> +        if review_request_draft.get_latest_diffset():
> -        diffset_count = review_request.diffset_history.diffsets.count()
> +            diffset_count = review_request.diffset_history.diffsets.count()
> -        if diffset_count < 1:
> +            if diffset_count < 1:
> -            # We don't need the first line, since it is also the attachment
> +                # We don't need the first line, since it is also the attachment
> -            # summary, which is displayed in the comment.
> +                # summary, which is displayed in the comment.
> -            extended_commit_msg = review_request_draft.description.partition(
> +                ext_commit_msg = review_request_draft.description.partition(

I don't really like this change, and I'm a little confused as to why you made it? to me `ext_commit_msg` is pretty ambiguous: external? extra? extended? etc.

I'd be open to renaming this something like `full_commit_msg` or `complete_commit_msg`.

Comment 27

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29809/#review36745

> I don't really like this change, and I'm a little confused as to why you made it? to me `ext_commit_msg` is pretty ambiguous: external? extra? extended? etc.
> 
> I'd be open to renaming this something like `full_commit_msg` or `complete_commit_msg`.

Heh I made it solely because 'extended_commit_msg' pushed the line past 80 chars and any other way of fixing it was super ugly.  full_commit_msg wfm though.

Comment 28

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/5-6/

Comment 29

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/6-7/

Comment 30

[Mark Côté [:mcote]]

Comment on attachment 8729128 [details]
MozReview Request: testing: Set mozreview_app_id in BMO params (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39255/diff/2-3/

Comment 31

[Mark Côté [:mcote]]

Comment on attachment 8729129 [details]
MozReview Request: testing: Optionally set BMO API key description (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39257/diff/2-3/

Comment 32

[Mark Côté [:mcote]]

Comment on attachment 8730322 [details]
MozReview Request: mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39797/diff/1-2/

Comment 33

[Steven MacLeod [:smacleod]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/29811/#review36749

::: pylib/mozreview/mozreview/bugzilla/client.py:53
(Diff revision 6)
>              raise BugzillaError('IOError: %s' % msg)
>      return _transform_errors
>  
>  
> +class BugzillaAttachmentUpdates(object):
> +

remove this blank.

::: pylib/mozreview/mozreview/bugzilla/client.py:70
(Diff revision 6)
> +        self.bug_id = bug_id
> +        self.attachments = []
> +        self.updates = []
> +        self.creates = []
> +
> +    @xmlrpc_to_bugzilla_errors

from what I can see we only hit bugzilla here using `self._update_attachments`, which already has this decorator. We should be able to remove it from this method.

::: pylib/mozreview/mozreview/bugzilla/client.py:168
(Diff revision 6)
> +        if rb_attachment:
> +            self.updates.append(params)
> +        else:
> +            self.creates.append(params)
> +
> +    @xmlrpc_to_bugzilla_errors

again, this can be removed.

Comment 34

[Steven MacLeod [:smacleod]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/29809/#review37317

Comment 35

[Steven MacLeod [:smacleod]]

Comment on attachment 8729128 [details]
MozReview Request: testing: Set mozreview_app_id in BMO params (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/39255/#review37333

Comment 36

[Steven MacLeod [:smacleod]]

Comment on attachment 8729129 [details]
MozReview Request: testing: Optionally set BMO API key description (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/39257/#review37337

::: testing/vcttesting/mozreview_mach_commands.py:259
(Diff revision 3)
> -    def create_api_key(self, where, email):
> -        mr = self._get_mozreview(where)
> +    @CommandArgument('--mozreview', action='store_true',
> +                     help='Make this a MozReview auth-delegation key')

Why not allow specifying a custom description here? not that we exactly need it, but we're allowing it in the other testing code.

Comment 37

[Steven MacLeod [:smacleod]]

Comment on attachment 8730322 [details]
MozReview Request: mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/39797/#review37341

::: pylib/mozreview/mozreview/bugzilla/client.py:174
(Diff revision 2)
>          """Mark any attachments for a given bug and review request as obsolete.
>  
>          This is called when review requests are discarded or deleted. We don't
>          want to leave any lingering references in Bugzilla.
>          """
>          params = {'ids': [], 'is_obsolete': True}

You're no longer using this, kill it.

::: pylib/mozreview/mozreview/bugzilla/client.py:182
(Diff revision 2)
>                      not a.get('is_obsolete')):
> -                params['ids'].append(a['id'])
> -
> -        if params['ids']:
> -            logger.info('Obsoleting attachments on bug %d: %s' % (
> +                logger.info('Obsoleting attachments on bug %d: %s' % (
> -                        self.bug_id, params['ids']))
> +                    self.bug_id, params['ids']))

this `params['ids']` is empty always now.

::: pylib/mozreview/mozreview/bugzilla/client.py:200
(Diff revision 2)
> +        # If there are attachments that didn't get created or updated, log an
> +        # error, but we shouldn't abort the publish because the the rest
> +        # completed.

How can this happen? have you seen it happen?

::: pylib/mozreview/mozreview/bugzilla/client.py:213
(Diff revision 2)
> +            logger.error('Tried to create %s attachments but %s reported as '
> +                         'created.' % (num_to_create, num_created))

Again, why would this work? I feel like the bugzilla API should be atomic and fail to publish anything if it can't do all of our creates?

Comment 38

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/29811/#review36749

> from what I can see we only hit bugzilla here using `self._update_attachments`, which already has this decorator. We should be able to remove it from this method.

Indeed, and actually _update_attachments() only calls Bugzilla.get_rb_attachments(), which has the decorator, so I removed it there as well.

Comment 39

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/39257/#review37337

> Why not allow specifying a custom description here? not that we exactly need it, but we're allowing it in the other testing code.

True, I wanted to keep it simple, but there is indeed a mismatch there.

Comment 40

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/39797/#review37341

> How can this happen? have you seen it happen?

Dylan says it shouldn't, that any failure to create or update an attachment will fail.  I saw that the method returns the IDs, though, and figured that an extra check can't hurt, in case something unexpected ever happens.

Comment 41

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/39797/#review37341

> Dylan says it shouldn't, that any failure to create or update an attachment will fail.  I saw that the method returns the IDs, though, and figured that an extra check can't hurt, in case something unexpected ever happens.

please expand the comment to include this information :)

Comment 42

[Steven MacLeod [:smacleod]]

Comment on attachment 8730322 [details]
MozReview Request: mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod

https://reviewboard.mozilla.org/r/39797/#review37399

With the comment fix discussed in my previous review we should be good to go.

Comment 43

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/39797/#review37341

> please expand the comment to include this information :)

Done.  I also moved the code that calculates ids_to_update to below the comment for clarity (and since the code that calculates num_to_create/num_created is also below).

Comment 44

[Mark Côté [:mcote]]

Comment on attachment 8704935 [details]
MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29809/diff/6-7/

Comment 45

[Mark Côté [:mcote]]

Comment on attachment 8704936 [details]
MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29811/diff/7-8/

Comment 46

[Mark Côté [:mcote]]

Comment on attachment 8729128 [details]
MozReview Request: testing: Set mozreview_app_id in BMO params (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39255/diff/3-4/

Comment 47

[Mark Côté [:mcote]]

Comment on attachment 8729129 [details]
MozReview Request: testing: Optionally set BMO API key description (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39257/diff/3-4/

Comment 48

[Mark Côté [:mcote]]

Comment on attachment 8730322 [details]
MozReview Request: mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39797/diff/2-3/

Comment 49

[Mark Côté [:mcote]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/7a1d74e36ee9
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6e773a88b955
https://hg.mozilla.org/hgcustom/version-control-tools/rev/fc1d115ff764
https://hg.mozilla.org/hgcustom/version-control-tools/rev/0555863c0d64
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c1d9267deb49

Should be deployed on Friday or Monday.