Gerrit is super spammy on large CLs |
|||||||||||||||
Issue descriptionI was spammed over 100 emails when uploading https://chromium-review.googlesource.com/472192. I think this is because Gerrit was adding the CCs one by one, which triggers a notification each time...
,
Apr 10 2017
Possible dupe of 5931.
,
Apr 10 2017
From NoteDb I see that these CCs were added one-by-one, so from what I see sending a notification for each of those operations is intended behavior. If this is not desired, the reviewers/CCs could be added in one operation (instead of individually) or 'notify' could be set to NONE in the API call. I'm adding tandrii@ here to shed some light on how the client part of the Chromium infra is making these calls.
,
Apr 10 2017
Issue 5931 may very well be part of it, but I'm also curious about how these CCs are being added. Moving to the chromium project for now. Is this "Blink Reformat" user automated? It looks like they added CCs across dozens of requests over the course of several minutes. What's going on in this interaction? How is it meant to operate? Will bringing 100+ emails down to 26 be sufficient?
,
Apr 10 2017
,
Apr 10 2017
Issue gerrit:5970 has been merged into this issue.
,
Apr 10 2017
,
Apr 10 2017
While the generation of the CL was automated, any CCs are added by 'git cl upload --gerrit', which is how code reviews in Chromium are pushed out of review.
,
Apr 10 2017
Actually, even for small CLs, it's spammy. I uploaded a new patchset to https://chromium-review.googlesource.com/473367 with git cl upload and got 18 messages in my inbox!
,
Apr 11 2017
+agable@ IIRC: CCs are added using API calls **after** "git push refs/for/refs.... because at the time CC weren't often working and if done using magic ref (...&cc=x@y.z) the whole change creation would fail. This is also why it's done using multiple calls and not one. In the future, please add agable@ instead of me.
,
Apr 11 2017
See https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?q=gerrit_util.AddReviewers+package:%5Echromium$&l=3041 which calls https://cs.chromium.org/chromium/tools/depot_tools/gerrit_util.py?l=683 The reason we set notify=bool(send_mail) is because we want cc-ed people to actually get email. However, notify= param in gerrit's rest api has just 4 options: NONE,ALL,OWNER,OWNER+REVIEWERS, and so we had to use ALL. However, I can see now that there is notify_details, so this can be improved https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#reviewer-input
,
Apr 11 2017
So, apparently `notify_details` has been there for a while, and so can already be used. However, it takes account ids (integers, not emails) and unregistered accounts have none, so we can't yet use this :(
,
Apr 11 2017
After discussion in IM with hiesel@: We can switch to use SetReview rest api insetad of AddReviewer. SetReview also allows to add reviewers, essentially in a batch. The ReviewerInput https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#review-input which has (currently undocumented) field to add reviewers (internal codesearch link http://shortn/_MQfblbuumq). Then, git cl upload would: 1. do not notify when creating a change: git push refs/for/refs/...&r=x,notify=NONE 2. add all CCs (if any) and notify using REST api SetReview described above. Git cl upload almost always has cc-ed users, and as such (2.) is necessary anyway, and will be faster than several REST api calls in sequence we do today. Ultimately, once unregistered cc emails are supported from git push call, we can combine 1 and 2.
,
Apr 13 2017
Can cc= be set in the initial push, just like r= is?
,
Apr 14 2017
,
Apr 14 2017
Issue gerrit:6007 has been merged into this issue.
,
Apr 17 2017
I have some CLs to fix this: https://chromium-review.googlesource.com/c/479450/ https://chromium-review.googlesource.com/c/479712/ We can't land the second one yet because it is blocked by a gerrit bug:
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/6d7ab1bfe53718fba047a876b659d17c43390a15 commit 6d7ab1bfe53718fba047a876b659d17c43390a15 Author: Aaron Gable <agable@chromium.org> Date: Tue Apr 18 18:37:12 2017 Refactor ReadHttpResponse to be error-friendlier Gerrit sometimes returns a full response json object at the same time as returning a non-200 status code. This refactor makes it easier for calling code to request access to that object and handle error cases on its own. Bug: 710028 Change-Id: Id1017d580d2fb843d5ca6287efcfed8775c52cd6 Reviewed-on: https://chromium-review.googlesource.com/479450 Commit-Queue: Aaron Gable <agable@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> [modify] https://crrev.com/6d7ab1bfe53718fba047a876b659d17c43390a15/presubmit_support.py [modify] https://crrev.com/6d7ab1bfe53718fba047a876b659d17c43390a15/git_cl.py [modify] https://crrev.com/6d7ab1bfe53718fba047a876b659d17c43390a15/tests/git_cl_test.py [modify] https://crrev.com/6d7ab1bfe53718fba047a876b659d17c43390a15/testing_support/gerrit_test_case.py [modify] https://crrev.com/6d7ab1bfe53718fba047a876b659d17c43390a15/gerrit_util.py
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/382674be12266d8155897a92a90d3760e478f6e5 commit 382674be12266d8155897a92a90d3760e478f6e5 Author: Aaron Gable <agable@chromium.org> Date: Tue Apr 18 18:50:24 2017 Revert "Refactor ReadHttpResponse to be error-friendlier" This reverts commit 6d7ab1bfe53718fba047a876b659d17c43390a15. Reason for revert: Stacktrace: File "/s/depot_tools/gerrit_util.py", line 816, in GetAccountDetails return ReadHttpJsonResponse(conn) File "/s/depot_tools/gerrit_util.py", line 376, in ReadHttpJsonResponse fh = ReadHttpResponse(conn, accept_statuses) File "/s/depot_tools/gerrit_util.py", line 365, in ReadHttpResponse if response.status not in accept_statuses: TypeError: argument of type 'NoneType' is not iterable Original change's description: > Refactor ReadHttpResponse to be error-friendlier > > Gerrit sometimes returns a full response json object at > the same time as returning a non-200 status code. This > refactor makes it easier for calling code to request > access to that object and handle error cases on its own. > > Bug: 710028 > Change-Id: Id1017d580d2fb843d5ca6287efcfed8775c52cd6 > Reviewed-on: https://chromium-review.googlesource.com/479450 > Commit-Queue: Aaron Gable <agable@chromium.org> > Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> > TBR=agable@chromium.org,tandrii@chromium.org,chromium-reviews@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: Ia9d9ce835e207a32e7cc8ee35c0cf40c823c7b78 Reviewed-on: https://chromium-review.googlesource.com/481059 Reviewed-by: Aaron Gable <agable@chromium.org> Commit-Queue: Aaron Gable <agable@chromium.org> [modify] https://crrev.com/382674be12266d8155897a92a90d3760e478f6e5/presubmit_support.py [modify] https://crrev.com/382674be12266d8155897a92a90d3760e478f6e5/git_cl.py [modify] https://crrev.com/382674be12266d8155897a92a90d3760e478f6e5/tests/git_cl_test.py [modify] https://crrev.com/382674be12266d8155897a92a90d3760e478f6e5/testing_support/gerrit_test_case.py [modify] https://crrev.com/382674be12266d8155897a92a90d3760e478f6e5/gerrit_util.py
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/19ee16c86067109d24303736f6ef53b95e2dfe30 commit 19ee16c86067109d24303736f6ef53b95e2dfe30 Author: Aaron Gable <agable@chromium.org> Date: Tue Apr 18 21:36:01 2017 Reland "Refactor ReadHttpResponse to be error-friendlier" Gerrit sometimes returns a full response json object at the same time as returning a non-200 status code. This refactor makes it easier for calling code to request access to that object and handle error cases on its own. The original version of this commit had a bug where ReadHttpResponse properly set the default value for accept_statuses, but all calls which came through ReadHttpJsonResponse were setting None instead. Bug: 710028 Change-Id: I8cee435d8acd487fb777b3fd69b5e48e19d2e5a3 Reviewed-on: https://chromium-review.googlesource.com/481060 Reviewed-by: Robbie Iannucci <iannucci@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> Commit-Queue: Aaron Gable <agable@chromium.org> [modify] https://crrev.com/19ee16c86067109d24303736f6ef53b95e2dfe30/presubmit_support.py [modify] https://crrev.com/19ee16c86067109d24303736f6ef53b95e2dfe30/git_cl.py [modify] https://crrev.com/19ee16c86067109d24303736f6ef53b95e2dfe30/tests/git_cl_test.py [modify] https://crrev.com/19ee16c86067109d24303736f6ef53b95e2dfe30/testing_support/gerrit_test_case.py [modify] https://crrev.com/19ee16c86067109d24303736f6ef53b95e2dfe30/gerrit_util.py
,
Apr 21 2017
,
Apr 27 2017
,
May 2 2017
,
May 4 2017
I'll land the CL under review when vi/gerritcodereview/release shows 1422 across the board.
,
May 8 2017
Issue gerrit:6152 has been merged into this issue.
,
May 9 2017
Issue gerrit:6168 has been merged into this issue.
,
May 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4 commit 6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4 Author: Aaron Gable <agable@chromium.org> Date: Tue May 09 21:47:36 2017 git-cl-upload: Set all reviewers and ccs in a single batch api call The old system had two faults: * It set reviewers and ccs via different mechanisms, which is confusing * It set CCs with a single call for each, resulting in N separate emails, with each email going to the uploader and all reviewers and all previous CCs. This new system just collects all reviewers and CCs, and sets them in a single call. That call will fail if *any* of the individual reviewers or ccs fail, so it also parses the response and retries with only the ones which would have succeeded on their own. If that second call fails, or the first fails in an unexpected way, it raises an exception like normal Bug: 710028 Change-Id: I1be508487a41f0b68f9c41908229b8f5342830a3 Reviewed-on: https://chromium-review.googlesource.com/479712 Commit-Queue: Aaron Gable <agable@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> [modify] https://crrev.com/6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4/tests/git_cl_test.py [modify] https://crrev.com/6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4/git_cl.py [modify] https://crrev.com/6dadfbfcf77356b3cf4b972de93c0ba1f9472fe4/gerrit_util.py
,
May 9 2017
As of now, only one message will be sent no matter how many people are added to the reviewers or CC lists.
,
May 11 2017
I don't think this is actually fixed. https://chromium-review.googlesource.com/c/502347/ was created today and still sent out multiple emails.
,
May 11 2017
That's because dcheng hasn't gotten the latest version of depot_tools yet. It will update the next time they run 'gclient sync'.
,
May 12 2017
Apart from the issue of CCs, there are emails sent that seem fundamentally like spam to me. The emails I've been seeing on gerrit reviews are of 3 forms: (1) "X has uploaded this change for review" (2) "X has posted comments on this change" (2) "X would like Y to review this change" Am I mistaken in thinking that emails of type (1) don't correspond to any emails sent out in the rietveld flow?
,
May 12 2017
You're not mistaken at all, those emails weren't present in Rietveld. But, those emails should also be removed by the CL above which fixed the CC spam. Please let us know if you're still receiving those emails after the git-cl fix has rolled out to everyone. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by dcheng@chromium.org
, Apr 10 2017