Speeding up change reindexing?

[Luca Milanesio]

Hi Gerrit contributors,
I was looking at a way to speedup the changes reindexing ... and I have actually find out that (for us) the slowest operation is the actual evaluation of the protocol rules for each individual change.

The relevant stack trace is:

at com.google.gerrit.server.project.ChangeControl.getRange(ChangeControl.java:326)
at gerrit.PRED__user_label_range_4.exec(PRED__user_label_range_4.java:76)
at com.googlecode.prolog_cafe.lang.PrologControl.executePredicate(PrologControl.java:191)
at com.googlecode.prolog_cafe.lang.BufferingPrologControl.run(BufferingPrologControl.java:147)
at com.googlecode.prolog_cafe.lang.BufferingPrologControl.all(BufferingPrologControl.java:121)
at com.google.gerrit.server.project.SubmitRuleEvaluator.evaluateImpl(SubmitRuleEvaluator.java:488)
at com.google.gerrit.server.project.SubmitRuleEvaluator.evaluate(SubmitRuleEvaluator.java:237)
at com.google.gerrit.server.query.change.ChangeData.submitRecords(ChangeData.java:1029)
at com.google.gerrit.server.index.change.ChangeField.storedSubmitRecords(ChangeField.java:921)
at com.google.gerrit.server.index.change.ChangeField.access$700(ChangeField.java:87)
at com.google.gerrit.server.index.change.ChangeField$40.get(ChangeField.java:884)
at com.google.gerrit.server.index.change.ChangeField$40.get(ChangeField.java:880)
at com.google.gerrit.server.index.Schema$1.apply(Schema.java:196)
at com.google.gerrit.server.index.Schema$1.apply(Schema.java:191)
at com.google.common.collect.Iterators$7.transform(Iterators.java:750)
at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:47)
at com.google.common.collect.Iterators$6.computeNext(Iterators.java:616)
at com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:145)
at com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:140)
at com.google.gerrit.lucene.AbstractLuceneIndex.toDocument(AbstractLuceneIndex.java:297)
at com.google.gerrit.lucene.LuceneChangeIndex.replace(LuceneChangeIndex.java:220)
at com.google.gerrit.lucene.LuceneChangeIndex.replace(LuceneChangeIndex.java:108)
at com.google.gerrit.server.index.change.ChangeIndexer.index(ChangeIndexer.java:205)

My question is: why do we need to store that user-specific information into our change index?
The index is shared across all users, why should I evaluate the rules for a specific user then?

Additionally, when performing on-line indexing, the group backends aren't available either, so the evaluation wouldn't succeed anyway.

I was tempted to just remove that info from the index and push for review :-)

Any feedback is highly appreciated.

Luca.

[lucamilanesio]

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user or not is not an property of the change but rather of the tuple (change, user).

Does it make sense to store that info into the index at all?

Luca.

[1] https://gerrit-review.googlesource.com/c/86807/

[Björn Pedersen]

Hi,

I think this thread [1] "Checking permissions while running reindex program?"
discussed this as well.

[1]https://groups.google.com/forum/#!searchin/repo-discuss/reindex|sort:relevance/repo-discuss/PaIjMyc9NlY/aQDOD2MdAAAJ

Björn

[Edwin Kempin]

On Fri, Dec 9, 2016 at 2:36 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user

It's not telling if the change submittable by a certain user.

The submit records just tell you whether the change fulfills all pre-conditions to be submittable (if it has all required approvals + no veto votes according to the submit rules). It doesn't take any submit permissions into account.

I believe when computing the submit records it checks for existing approvals whether the user that gave the approval still has permissions to give that approval. E.g. if a user has approved a change, but then lost the permission to approve, this approval is filtered out.

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[lucamilanesio]

On Friday, December 9, 2016 at 1:44:33 PM UTC, Edwin Kempin wrote:

On Fri, Dec 9, 2016 at 2:36 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user

It's not telling if the change submittable by a certain user.

The submit records just tell you whether the change fulfills all pre-conditions to be submittable (if it has all required approvals + no veto votes according to the submit rules). It doesn't take any submit permissions into account.

But from what I've seen in the code stack trace, the prolog rule is actually going to check the current user's permissions on the required review labels: the permissions validation and associated results are dependent on the current user and may give different values by different users.

I believe this was exactly the discussion thread pointed out by Bjorn were Saša had similar concerns.

Additionally, the current user permissions and ability to access the labels may be completely wrong during off-line reindexing (e.g. we don't load plugins and thus we don't evaluate for instance the user's LDAP groups ownership), giving as a result that is potentially incorrect.

The off-line and on-line reindexing would generate different values as a result of that.

 

--
--
To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

[Dave Borowitz]

On Fri, Dec 9, 2016 at 8:58 AM, lucamilanesio <luca.mi...@gmail.com> wrote:

On Friday, December 9, 2016 at 1:44:33 PM UTC, Edwin Kempin wrote:

On Fri, Dec 9, 2016 at 2:36 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user

It's not telling if the change submittable by a certain user.

The submit records just tell you whether the change fulfills all pre-conditions to be submittable (if it has all required approvals + no veto votes according to the submit rules). It doesn't take any submit permissions into account.

But from what I've seen in the code stack trace, the prolog rule is actually going to check the current user's permissions on the required review labels: the permissions validation and associated results are dependent on the current user and may give different values by different users.

It doesn't depend on the calling user. It's going through each PatchSetApproval and checking whether *the user who gave that approval* still has permission to give that approval.

(PRED__user_label_range_4 takes as one of its arguments a CurrentUser, but that's the Gerrit CurrentUser type, which just means "some user". The actual CurrentUser object passed in varies depending on the PSA being inspected.)

 

I believe this was exactly the discussion thread pointed out by Bjorn were Saša had similar concerns.

Yeah but the conclusion from that thread was that squashing votes based on *current* permissions is an important feature, so we couldn't really change it, so here we are.

Maybe we can optimize the permissions checking? Or, just for offline reindexing, have a cache of (account ID, dest ref, label) -> permission range?

 

Additionally, the current user permissions and ability to access the labels may be completely wrong during off-line reindexing (e.g. we don't load plugins and thus we don't evaluate for instance the user's LDAP groups ownership), giving as a result that is potentially incorrect.

I'm not sure I follow. What plugins can affect LDAP groups?

 

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[Dave Borowitz]

On Fri, Dec 9, 2016 at 9:30 AM, Dave Borowitz <dbor...@google.com> wrote:

On Fri, Dec 9, 2016 at 8:58 AM, lucamilanesio <luca.mi...@gmail.com> wrote:

On Friday, December 9, 2016 at 1:44:33 PM UTC, Edwin Kempin wrote:

On Fri, Dec 9, 2016 at 2:36 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user

It's not telling if the change submittable by a certain user.

The submit records just tell you whether the change fulfills all pre-conditions to be submittable (if it has all required approvals + no veto votes according to the submit rules). It doesn't take any submit permissions into account.

But from what I've seen in the code stack trace, the prolog rule is actually going to check the current user's permissions on the required review labels: the permissions validation and associated results are dependent on the current user and may give different values by different users.

It doesn't depend on the calling user. It's going through each PatchSetApproval and checking whether *the user who gave that approval* still has permission to give that approval.

(PRED__user_label_range_4 takes as one of its arguments a CurrentUser, but that's the Gerrit CurrentUser type, which just means "some user". The actual CurrentUser object passed in varies depending on the PSA being inspected.)

 

I believe this was exactly the discussion thread pointed out by Bjorn were Saša had similar concerns.

Yeah but the conclusion from that thread was that squashing votes based on *current* permissions is an important feature, so we couldn't really change it, so here we are.

Maybe we can optimize the permissions checking? Or, just for offline reindexing, have a cache of (account ID, dest ref, label) -> permission range?

 

Additionally, the current user permissions and ability to access the labels may be completely wrong during off-line reindexing (e.g. we don't load plugins and thus we don't evaluate for instance the user's LDAP groups ownership), giving as a result that is potentially incorrect.

I'm not sure I follow. What plugins can affect LDAP groups?

Anyway, I'm not really interested in the specifics here. If the set of loaded plugins can affect the results of indexing, then offline reindexing needs to load plugins. 

[Luca Milanesio]

On 9 Dec 2016, at 14:30, Dave Borowitz <dbor...@google.com> wrote:

On Fri, Dec 9, 2016 at 8:58 AM, lucamilanesio <luca.mi...@gmail.com> wrote:

On Friday, December 9, 2016 at 1:44:33 PM UTC, Edwin Kempin wrote:

On Fri, Dec 9, 2016 at 2:36 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

So, it seems that it has been recently introduced by Dave in [1] to speed-up the Queries like "is:submittable" (see [1]).

However, this may be really misleading because if you imagine this scenario:

- I am a user without the *full power* on a change C

- I am posting a score on C which update its status

The fact of being "submittable" by that user

It's not telling if the change submittable by a certain user.

The submit records just tell you whether the change fulfills all pre-conditions to be submittable (if it has all required approvals + no veto votes according to the submit rules). It doesn't take any submit permissions into account.

But from what I've seen in the code stack trace, the prolog rule is actually going to check the current user's permissions on the required review labels: the permissions validation and associated results are dependent on the current user and may give different values by different users.

It doesn't depend on the calling user. It's going through each PatchSetApproval and checking whether *the user who gave that approval* still has permission to give that approval.

Gotcha, I was mislead by the class name :-)

(PRED__user_label_range_4 takes as one of its arguments a CurrentUser, but that's the Gerrit CurrentUser type, which just means "some user". The actual CurrentUser object passed in varies depending on the PSA being inspected.)

OK, that makes much more sense.

 

I believe this was exactly the discussion thread pointed out by Bjorn were Saša had similar concerns.

Yeah but the conclusion from that thread was that squashing votes based on *current* permissions is an important feature, so we couldn't really change it, so here we are.

Maybe we can optimize the permissions checking?

Or, just for offline reindexing, have a cache of (account ID, dest ref, label) -> permission range?

Yes, that would definitely help.

 

Additionally, the current user permissions and ability to access the labels may be completely wrong during off-line reindexing (e.g. we don't load plugins and thus we don't evaluate for instance the user's LDAP groups ownership), giving as a result that is potentially incorrect.

I'm not sure I follow. What plugins can affect LDAP groups?

Sorry, I said plugin but in that case is simply the Guice binding that is needed, as LDAP is internal.

Other group backends are provided by plugins:

- singleusergroup

- github

The off-line reindexing would be faster but incorrect, as the permissions used for the calculation would have been mostly incomplete.

[Dave Borowitz]

We've been talking about this a bit internally, and we have come to the tentative decision to just kill the whole retroactive vote-squashing "feature" entirely, both during indexing and in the frontend. AFAIK that is the only way that permission evaluation should be happening during reindexing.

Vote squashing has quite a few downsides:
* It doesn't (and can't feasibly) mention in the change message log that this has happened.
* Thus there may be an inconsistency in the UI between the value on the reviewer chip and the value in the message.
* The submit rule evaluator has confusing notions of strict/slow vs. lenient/fast evaluations scattered throughout.
* Permissions checks are either slow during batch indexing (aka the original complaint on this thread).

The upsides are...slimmer. Basically it feels like we have been doing this forever just because we can.

Arguably, admins revoking a voting permission want to be able to limit the damage of an unintentionally broad permission. But:
* We never really polled anyone to determine whether they actually do care or expect it to work this way.
* The threat model is preventing submission of changes by people who shouldn't have permission. If someone has CR+2 permission, they probably also have Submit. Any reasonably determined attacker would just CR+2 and submit immediately, which retroactively squashing votes wouldn't prevent.
* No other permission (other than Read) has this kind of retroactive behavior.

So I say, let's just kill it. We can delete code, unconfuse users, and speed up indexing with one fell swoop.

Any objections?

-- 
-- 
To unsubscribe, email repo-discuss+unsub...@googlegroups.com

[Martin Fick]

On Wednesday, March 08, 2017 02:11:58 PM 'Dave Borowitz' via

Yes! -2

I believe vote squashing is very important in general, we
change policies about what can be merged on branches, and
who can decide that, all the time approaching releases.
Eliminating vote squashing would likely be devastating to
this use case.

We also use vote squashing, and in some more custom use
cases that may not apply upstream yet. :) However, before
I bother explaining those, it is important to point out
another current use case that this affects: moving a change
to a new destination branch.

On the source branch, a reviewer may have had permissions to
+2, on the destination branch they may not. Vote squashing
will potentially turn their +2 into a +1 on the new
destination branch, it is important that it not be a +2 on
the new destination branch or users would be able to use
change moving to grant themselves +2 on branches where they
do not have it.

The second part of moving that can be problematic, is moving
a change back to the original source branch. If the change
originally had a -2, and then it gets squahed to -1 by
moving it to a new destination branch, it might be
convenient to think "no big deal we are going to reindex the
change on move anyway, so squash the vote then for real".
But the problem with that approach is when the user now
moves it back to the original source branch, the -1 needs to
become unsquashed, and go back to -2, otherwise users can
use moving a change to remove -2s from their change!


-Martin

--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation

[Dave Borowitz]

Damn. I forgot that moving changes is a thing now. We certainly can't allow removing -2s via moving.

[Shawn Pearce]

Damn, Martin presents really compelling uses for vote squashing. I really was hoping we could kill it, but Martin's right, we can't.

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

[Shawn Pearce]

Actually...

What about adding a new label to release branches via Prolog rules?

Code-Review+2 stays unchanged, but when a branch nears release an
additional label is required to submit. That doesn't require squashing
to downgrade +2 -> +1.

>>> We also use vote squashing, and in some more custom use
>>> cases that may not apply upstream yet. :) However, before
>>> I bother explaining those, it is important to point out
>>> another current use case that this affects: moving a change
>>> to a new destination branch.
>>>
>>> On the source branch, a reviewer may have had permissions to
>>> +2, on the destination branch they may not. Vote squashing
>>> will potentially turn their +2 into a +1 on the new
>>> destination branch, it is important that it not be a +2 on
>>> the new destination branch or users would be able to use
>>> change moving to grant themselves +2 on branches where they
>>> do not have it.

As you mention, we could squash the vote during the move step. There
are already a number of things going on there that the squash could
happen at move time, rather than on the fly during every access.

>>> The second part of moving that can be problematic, is moving
>>> a change back to the original source branch. If the change
>>> originally had a -2, and then it gets squahed to -1 by
>>> moving it to a new destination branch, it might be
>>> convenient to think "no big deal we are going to reindex the
>>> change on move anyway, so squash the vote then for real".

I think you can just do that. Squash the vote from during the move, if
the reviewer lacks +2 on the new destination branch.

>>> But the problem with that approach is when the user now
>>> moves it back to the original source branch, the -1 needs to
>>> become unsquashed, and go back to -2, otherwise users can
>>> use moving a change to remove -2s from their change!

When going back to the original branch, we have NoteDb. We have history!

If we can identify that the current score of -1 was caused by an
auto-squash during move, we can dig further through history and find
the prior value. That may be some work for NoteDb to reassemble the
history of every label score throughout time and allow the branch move
operation to find the "correct" most recent value and restore it, but
the cost of that would be restricted to the branch move operation and
not everything that runs everywhere in Gerrit.

[Luca Milanesio]

My original complain wasn't just about speed but even accuracy !

When we do vote squashing during off-line reindexing, we do not have a "whole Gerrit" up-and-running and we don't have the plugins either.
(core or non-core)

One of the most popular plugin is singleusergroup and allows to define ACLs on users, including permissions to Code-Review and Submit, quite important for an accurate vote-squashing feature ;-)
On GerritHub we rely as well on GitHub Organisation/Team ownership to evaluate ACLs, and those are not used at all during off-line reindex.

Bottom line is: vote-squashing during off-line reindexing is not only slow, but even incorrect and misleading :-(
In real terms, even if it were accurate, the speed issue is a show-stopper and it is not incremental either.

I see Martin's point and his use-case is very important: however I agree with Shawn that vote-squashing should be best performed *when* the change is moved.
Additionally, we could introduce a new REST-API (+ SSH command) to recalculate it ad-hoc when needed (e.g. when ACLs of a project are changed).

Luca.

[David Ostrovsky]

On Wednesday, March 8, 2017 at 8:12:28 PM UTC+1, Dave Borowitz wrote:

We've been talking about this a bit internally, and we have come to the tentative decision to just kill the whole retroactive vote-squashing "feature" entirely, both during indexing and in the frontend. AFAIK that is the only way that permission evaluation should be happening during reindexing.

Vote squashing has quite a few downsides:
* It doesn't (and can't feasibly) mention in the change message log that this has happened.
* Thus there may be an inconsistency in the UI between the value on the reviewer chip and the value in the message.
* The submit rule evaluator has confusing notions of strict/slow vs. lenient/fast evaluations scattered throughout.
* Permissions checks are either slow during batch indexing (aka the original complaint on this thread).

The upsides are...slimmer. Basically it feels like we have been doing this forever just because we can.

Arguably, admins revoking a voting permission want to be able to limit the damage of an unintentionally broad permission. But:
* We never really polled anyone to determine whether they actually do care or expect it to work this way.
* The threat model is preventing submission of changes by people who shouldn't have permission. If someone has CR+2 permission, they probably also have Submit. Any reasonably determined attacker would just CR+2 and submit immediately, which retroactively squashing votes wouldn't prevent.
* No other permission (other than Read) has this kind of retroactive behavior.

So I say, let's just kill it. We can delete code, unconfuse users, and speed up indexing with one fell swoop.

+1. I would even prefer to do that before cutting of 2.14 stable branch.

By all academic discussions about non-existing threat-models with imaginary

CR+2 attackers, or non-existing feature requests like votes recovery when

playing ping-pong games with "move changes across branches" feature, that

nobody using until now anyway, as it was only added in 2.13, and even wasn't

exposed through the UI yet, we must not forget that:

  First and foremost speed is not just a feature.

  Speed is the most important feature.

[Luca Milanesio]

+1

Yes, 100% agreed. When reindex takes hours (> 4h for GerritHub.io) it becomes so inaccessible that becomes useless.

I remember years ago that I was running a project for a large mobile operator and the "home page" was taking 60s to render ... but was "technically working fine".

The problem was raised as P1 because .... 90% of the people after waiting 30s just give up: *speed* is the most important feature of all, because makes features accessible.

The changes moved across branches is a good point but represents IMHO <1% of all the use-cases.

If we could "flag" the changes being moved and do a vote squashing just for those? 

Luca.

[Saša Živkov]

On Wed, Mar 8, 2017 at 8:11 PM, 'Dave Borowitz' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

We've been talking about this a bit internally, and we have come to the tentative decision to just kill the whole retroactive vote-squashing "feature" entirely, both during indexing and in the frontend. AFAIK that is the only way that permission evaluation should be happening during reindexing.

Vote squashing has quite a few downsides:
* It doesn't (and can't feasibly) mention in the change message log that this has happened.
* Thus there may be an inconsistency in the UI between the value on the reviewer chip and the value in the message.
* The submit rule evaluator has confusing notions of strict/slow vs. lenient/fast evaluations scattered throughout.
* Permissions checks are either slow during batch indexing (aka the original complaint on this thread).

The upsides are...slimmer. Basically it feels like we have been doing this forever just because we can.

Arguably, admins revoking a voting permission want to be able to limit the damage of an unintentionally broad permission.

This is only one use case.

Another possibility is that a voting permission is revoked from user because that user leaves the team/project.

In that case, any existing votes from this user (even +2 votes) are still valid as they were made during the

time the user was a member of the team/project.

When a voting permission is revoked from a user, Gerrit doesn't know the reason (too broad permission

or the user leaving the team). The admin revoking the permission can make a reasonable decision

what to do with the existing votes of that user, Gerrit server probably cannot.

 

-- 
-- 
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

[Saša Živkov]

Is this actually a case against squashing?

Is squashing without unsquashing broken?

 

-Martin

--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

[Dave Borowitz]

Yes. We have a permission controlling the ability to remove votes. Martin has described a way to remove votes without having that permission, which we can't allow.

One solution (that Martin might not like): don't preserve votes at all when moving changes. A +2 vote means "this code is OK to submit to this branch." If the change is no longer on the original branch, that statement no longer applies. 

[Dave Borowitz]

You're right, I don't think the NoteDb implementation would be that hard.

The ReviewDb implementation is not hard either. We just need another column in PatchSetApproval, originalValue. Squash during move modifies value but not originalValue; any other vote modifies both.

Supporting explicit squashing in this way also opens the door for a plugin to listen for refs/meta/config changes and manually squash votes for all potentially affected changes. This has obvious performance downsides, but they're limited to handling config changes.

[Saša Živkov]

Yes, this addresses the +2 issue.

However, the -2 issue would still exist.

I wonder how realistic it is to automatically squash a -2 to -1, anyway?

For example: a change is pushed to branch foo and gets -2. When that change

gets moved to another branch bar, how can Gerrit know if the -2 can be safely removed (squashed to -1)?

It may be that the reason for -2 was that the change was pushed to a wrong branch.

However, the reason for -2 may be independent of the change branch.

The reviewer who voted -2 knows the reason.

Gerrit doesn't know the reason, but it squashes the vote to make it compatible with the

access permissions in the target branch. This may confuse the users.

Would any user actually complain if the -2 wouldn't be squashed?

Having a -2 from a voter who cannot vote -2 in that branch, may still be a bit confusing but it can be easily

tracked by looking at the change history.

[Martin Fick]

I am changing this to a +1 (for many of the reasons
discussed by others)

I agree that these solutions would work well for most use
cases, and likely simplify understanding for users.

Users generally got confused by implicit vote squashing, so
explicity squashing is probably better if squashing is
desired (I am still a bit unsure about squashing due to
users changing groups though). As Saša pointed out, Gerrit
doesn't really know the reason for many squashes, so it may
not actually be doing the right thing in some cases.
Explicity squashing based policies can be implemented in
many ways by plugins if desired, and it might lead to a
clearer system in the end,

-Martin