New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 710028 link

Starred by 12 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: ----

Blocked on:
issue gerrit:6016

Blocking:
issue 715856


Previous locations:
gerrit:5968


Sign in to add a comment

Gerrit is super spammy on large CLs

Project Member Reported by dcheng@chromium.org, Apr 9 2017

Issue description

I 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...
 

Comment 1 by dcheng@chromium.org, Apr 10 2017

OK, even a smaller +2000/-2000 line CL ended up spamming me with 111 emails.

Comment 2 by wyatta@google.com, Apr 10 2017

Owner: hie...@google.com
Possible dupe of 5931.

Comment 3 by hie...@google.com, Apr 10 2017

Cc: tandrii@chromium.org
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.

Comment 4 by logan@google.com, Apr 10 2017

Components: -PolyGerrit
Labels: Proj-Gerrit-Migration
Owner: ----
 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?

Comment 5 by logan@google.com, Apr 10 2017

Project: chromium
Moved issue gerrit:5968 to now be  issue chromium:710028 .

Comment 6 by logan@google.com, Apr 10 2017

Labels: -Proj=Gerrit-Migration Proj-Gerrit-Migration
 Issue gerrit:5970  has been merged into this issue.

Comment 7 by wyatta@google.com, Apr 10 2017

Cc: wyatta@google.com
Components:

Comment 8 by dcheng@chromium.org, 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.

Comment 9 by dcheng@chromium.org, 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!
Cc: aga...@chromium.org
+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.
Components: Infra>Codereview>Gerrit Infra>SDK
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


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 :( 
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.

Comment 14 by jrn@google.com, Apr 13 2017

Can cc= be set in the initial push, just like r= is?

Comment 15 by wyatta@google.com, Apr 14 2017

Cc: hie...@google.com logan@google.com
 Issue 711745  has been merged into this issue.

Comment 16 by wyatta@google.com, Apr 14 2017

 Issue gerrit:6007  has been merged into this issue.
Blockedon: gerrit:6016
Owner: aga...@chromium.org
Status: Started (was: New)
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: 
Project Member

Comment 18 by bugdroid1@chromium.org, 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

Project Member

Comment 19 by bugdroid1@chromium.org, 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

Project Member

Comment 20 by bugdroid1@chromium.org, 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

Labels: Milestone-Launch
Blocking: 715856
Labels: Pri-1
I'll land the CL under review when vi/gerritcodereview/release shows 1422 across the board.

Comment 25 by wyatta@google.com, May 8 2017

 Issue gerrit:6152  has been merged into this issue.

Comment 26 by wyatta@google.com, May 9 2017

 Issue gerrit:6168  has been merged into this issue.
Project Member

Comment 27 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
As of now, only one message will be sent no matter how many people are added to the reviewers or CC lists.

Comment 29 by nasko@chromium.org, May 11 2017

Status: Unconfirmed (was: Fixed)
I don't think this is actually fixed. https://chromium-review.googlesource.com/c/502347/ was created today and still sent out multiple emails.
Status: Fixed (was: Unconfirmed)
That's because dcheng hasn't gotten the latest version of depot_tools yet. It will update the next time they run 'gclient sync'.
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?
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