Use new BMO MozReview batch-attachment API

RESOLVED FIXED

Status

MozReview
General
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcote, Assigned: mcote)

Tracking

(Blocks: 2 bugs)

Production
Dependency tree / graph

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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).
(Assignee)

Comment 1

2 years ago
Created attachment 8704935 [details]
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/
Attachment #8704935 - Flags: review?(smacleod)
(Assignee)

Comment 2

2 years ago
Created attachment 8704936 [details]
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/
Attachment #8704936 - Flags: review?(smacleod)
(Assignee)

Comment 3

2 years ago
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

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8704935 - Flags: review?(smacleod) → review?(dminor)
(Assignee)

Updated

2 years ago
Attachment #8704936 - Flags: review?(smacleod) → review?(dminor)

Comment 5

2 years ago
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
Attachment #8704935 - Flags: review?(dminor) → review+

Updated

2 years ago
Attachment #8704936 - Flags: review?(dminor)

Comment 6

2 years ago
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?
Blocks: 1246861
Product: Developer Services → MozReview
(Assignee)

Comment 7

2 years ago
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/
Attachment #8704935 - Attachment description: MozReview Request: rbbz: Use a single internal call for all attachment actions (bug 1237491). r?smacleod → MozReview Request: rbbz: Use a single internal call for all attachment actions (bug 1237491).
(Assignee)

Comment 8

2 years ago
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/
Attachment #8704936 - Attachment description: MozReview Request: rbbz: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod → MozReview Request: rbbz: Do all Bugzilla attachment actions together (bug 1237491).
Attachment #8704936 - Flags: review?(dminor)
(Assignee)

Updated

2 years ago
Attachment #8704936 - Flags: review?(dminor)
(Assignee)

Updated

2 years ago
Assignee: mcote → mdoglio
(Assignee)

Comment 9

2 years ago
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.
:mcote last thing we said is that you are finishing this, right?
Assignee: mdoglio → mcote
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
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/
Attachment #8704935 - Attachment description: MozReview Request: rbbz: Use a single internal call for all attachment actions (bug 1237491). → MozReview Request: mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod
Attachment #8704935 - Flags: review?(smacleod)
(Assignee)

Comment 14

2 years ago
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/
Attachment #8704936 - Attachment description: MozReview Request: rbbz: Do all Bugzilla attachment actions together (bug 1237491). → MozReview Request: mozreview: Do all Bugzilla attachment actions together (bug 1237491). r?smacleod
Attachment #8704936 - Flags: review?(smacleod)
(Assignee)

Comment 15

2 years ago
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/
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
Created attachment 8729128 [details]
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/
Attachment #8729128 - Flags: review?(smacleod)
(Assignee)

Comment 19

2 years ago
Created attachment 8729129 [details]
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/
Attachment #8729129 - Flags: review?(smacleod)
(Assignee)

Comment 20

2 years ago
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).
Summary: Consolidate attachment creation/update calls → Use new BMO MozReview batch-attachment API
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 23

2 years ago
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/
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
Created attachment 8730322 [details]
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/
Attachment #8730322 - Flags: review?(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`.
Attachment #8704935 - Flags: review?(smacleod)
(Assignee)

Comment 27

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8704935 - Flags: review?(smacleod)
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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/
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Comment 32

2 years ago
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 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.
Attachment #8704936 - Flags: review?(smacleod) → review+
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
Attachment #8704935 - Flags: review?(smacleod) → review+
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
Attachment #8729128 - Flags: review?(smacleod) → review+
Attachment #8729129 - Flags: review?(smacleod) → review+
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 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?
Attachment #8730322 - Flags: review?(smacleod)
(Assignee)

Comment 38

2 years ago
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.
(Assignee)

Comment 39

2 years ago
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.
(Assignee)

Comment 40

2 years ago
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.
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 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.
Attachment #8730322 - Flags: review+
(Assignee)

Comment 43

2 years ago
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).
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Comment 45

2 years ago
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/
(Assignee)

Comment 46

2 years ago
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/
(Assignee)

Comment 47

2 years ago
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/
(Assignee)

Comment 48

2 years ago
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/
(Assignee)

Comment 49

2 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.