Closed
Bug 1237491
Opened 8 years ago
Closed 8 years ago
Use new BMO MozReview batch-attachment API
Categories
(MozReview Graveyard :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcote, Unassigned)
References
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
dminor
:
review+
smacleod
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
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•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 years ago
|
Attachment #8704935 -
Flags: review?(smacleod) → review?(dminor)
Assignee | ||
Updated•8 years ago
|
Attachment #8704936 -
Flags: review?(smacleod) → review?(dminor)
Comment 5•8 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•8 years ago
|
Attachment #8704936 -
Flags: review?(dminor)
Comment 6•8 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?
Updated•8 years ago
|
Product: Developer Services → MozReview
Assignee | ||
Comment 7•8 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•8 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•8 years ago
|
Attachment #8704936 -
Flags: review?(dminor)
Assignee | ||
Updated•8 years ago
|
Assignee: mcote → mdoglio
Assignee | ||
Comment 9•8 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.
Comment 10•8 years ago
|
||
:mcote last thing we said is that you are finishing this, right?
Assignee: mdoglio → mcote
Assignee | ||
Comment 11•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
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 26•8 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/#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•8 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•8 years ago
|
Attachment #8704935 -
Flags: review?(smacleod)
Assignee | ||
Comment 28•8 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•8 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•8 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•8 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•8 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 33•8 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/#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 34•8 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/#review37317
Attachment #8704935 -
Flags: review?(smacleod) → review+
Comment 35•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8729129 -
Flags: review?(smacleod) → review+
Comment 36•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 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.
Comment 41•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 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•8 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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•