is:mergeable and dashboards in 3.4

[Jan Kundrát]

Hi,
I read [1] that the is:mergable operator got disabled by default (and we
bumped into this as users of GerritHub).

We've had this operator in our stored project dashboard which acted as a
landing page. Not a bit deal, we've updated [2] that one, but it's
something which raised a support request already.

For us, it was a piece of useful information. We created that dashboard as
a tool for users to take a peek at what is brewing in the repo. Our
dashboard was split into "what needs work from the original developer",
"what needs a reviewer's attention", and "what got done recently". Anything
which wouldn't merge went straight into the "needs a developer to finish"
bucket. This dashboard of course predates the excellent Attention Sets
which I personally just love, and there's a certain overlap in features.
Still, the attention set is inherently personalised, and it's an excellent
tool for active developers. What we solved with a dashboard is an overview
that centers around the full project, not around one particular developer.
I think I could get a similar result if there was a query operator such as
"is the change owner in the attention set", but I don't think there's an
operator like that.

The error message can be made clearer, e.g., by pointing to the release
notes. But perhaps that wouldn't help for users who obviously cannot
trigger it.

Finally, it looks like I don't understand the reasoning given in the docs.
My understanding of a change being "mergeable" is that it can be applied to
the current tip of the target branch, whatever it is right now. How does
that trigger an O(n^2) complexity per change? Isn't it at most
O(n_open_changes * n_diff_hunks_in_change)? Of course if this info goes
into the index, it's something like O(n_open_changes) complexity at push
time, but that's again just the cost of doing business, right? And even if
it O(n^2), well, what is the alternative? As a developer I surely hope I
see a red/green thingy indicating to me if I can merge that one change that
I'm looking at now. I do not want to find that out only after I've hit the
submit button. Is that going to be supported? And if it is, why not show
that info in the listing of changes as well?

Speaking about the dashboards and query results, originally these were
paged. Each of the independent queries showed something like "10 or more",
and you would only get the full list upon a click (and erven that was still
paginated). That's not the case on GerritHub right now, and perhaps a
reason why our dashboard [3] takes 10+ seconds to render now.

With kind regards,
Jan

[1]
https://www.gerritcodereview.com/3.4.html#ismergeable-predicate-is-disabled-per-default
[2]
https://github.com/Telecominfraproject/oopt-gnpy/commit/89b4da9518237e4a0902ead288a8c6049b4ae1f3
[3]
https://review.gerrithub.io/p/Telecominfraproject/oopt-gnpy/+/dashboard/main:main

[David Ostrovsky]

If I open your change: [1], I see red "Merge Conflict" label. Mergeable bit is

computing asynchronously during rendering of the change screen.

 

And if it is, why not show
that info in the listing of changes as well?

Because it's not a part of the secondary index any more. Change list and dashboards

are rendered using secondary index only, without consulting primary storage.

[1] https://review.gerrithub.io/c/Telecominfraproject/oopt-gnpy/+/479563

[Matthias Sohn]

I think the complexity is O((number of open changes for a branch)ˆ2).

If a new patchset is pushed for any of the open changes for a branch of a repository

mergeability must be recomputed against all other open changes for the same branch.

If there are 10 open changes for the repository foo and a new patchset

for one of these changes is uploaded Gerrit must run a git merge for the

9 other changes open for the same branch. 

If there 100 open changes for a branch then 99 merges must be executed

when 1 new patchset arrives. In addition the number of new patchsets 

arriving for the same branch will grow with the number of open changes for

that branch.

  

into the index, it's something like O(n_open_changes) complexity at push
time, but that's again just the cost of doing business, right? And even if
it O(n^2), well, what is the alternative? As a developer I surely hope I
see a red/green thingy indicating to me if I can merge that one change that
I'm looking at now. I do not want to find that out only after I've hit the
submit button. Is that going to be supported? And if it is, why not show
that info in the listing of changes as well?

Speaking about the dashboards and query results, originally these were
paged. Each of the independent queries showed something like "10 or more",
and you would only get the full list upon a click (and erven that was still
paginated). That's not the case on GerritHub right now, and perhaps a
reason why our dashboard [3] takes 10+ seconds to render now.

With kind regards,
Jan

[1]
https://www.gerritcodereview.com/3.4.html#ismergeable-predicate-is-disabled-per-default
[2]
https://github.com/Telecominfraproject/oopt-gnpy/commit/89b4da9518237e4a0902ead288a8c6049b4ae1f3
[3]
https://review.gerrithub.io/p/Telecominfraproject/oopt-gnpy/+/dashboard/main:main

--
--
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.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/c1c8de6c-d0c2-4f1d-9497-32444ec0e23a%40flaska.net.

[Martin Fick]

On Thursday, May 20, 2021 5:46:36 PM MDT Matthias Sohn wrote:
> On Thu, May 20, 2021 at 3:20 PM Jan Kundrát <j...@flaska.net> wrote:
> > Finally, it looks like I don't understand the reasoning given in the docs.
> > My understanding of a change being "mergeable" is that it can be applied
> > to
> > the current tip of the target branch, whatever it is right now. How does
> > that trigger an O(n^2) complexity per change? Isn't it at most
> > O(n_open_changes * n_diff_hunks_in_change)? Of course if this info goes
>
> I think the complexity is O((number of open changes for a branch)ˆ2).
>
> If a new patchset is pushed for any of the open changes for a branch of a
> repository
> mergeability must be recomputed against all other open changes for the same
> branch.
>
> If there are 10 open changes for the repository foo and a new patchset
> for one of these changes is uploaded Gerrit must run a git merge for the
> 9 other changes open for the same branch.
>
> If there 100 open changes for a branch then 99 merges must be executed
> when 1 new patchset arrives. In addition the number of new patchsets
> arriving for the same branch will grow with the number of open changes for
> that branch.

This sounds like you are talking about computing conflicts with other changes,
not whether changes are mergeable to the tip?

I think the other part overlooked is that presumably some of those changes get
merged to the tip, and when they do, all the open changes need to be re-
computed,

-Martin

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

[Matthias Sohn]

yep, you are right

[Luca Milanesio]

Hi Jan,

Thanks for your feedback, which is always very much appreciated.

Answers inline.

On 20 May 2021, at 14:20, Jan Kundrát <j...@flaska.net> wrote:

Hi,
I read [1] that the is:mergable operator got disabled by default (and we bumped into this as users of GerritHub).
We've had this operator in our stored project dashboard which acted as a landing page. Not a bit deal, we've updated [2] that one, but it's something which raised a support request already.

For us, it was a piece of useful information.

We are re-enabling the is:mergeable on GerritHub.io: by checking the system load, it is something we can still afford at the moment and we are happy to listen to our customers :-)

Current status:

- EU site is enabled again (review-eu.gerrithub.io)

- US site is in-progress (review-am.gerrithub.io)

review.gerrithub.io is going by default to the US site, so it will take a few more minutes.

P.S. GerritHub is multi-site, so all sites are replicated transparently.

We created that dashboard as a tool for users to take a peek at what is brewing in the repo. Our dashboard was split into "what needs work from the original developer", "what needs a reviewer's attention", and "what got done recently". Anything which wouldn't merge went straight into the "needs a developer to finish" bucket. This dashboard of course predates the excellent Attention Sets which I personally just love, and there's a certain overlap in features. Still, the attention set is inherently personalised, and it's an excellent tool for active developers. What we solved with a dashboard is an overview that centers around the full project, not around one particular developer. I think I could get a similar result if there was a query operator such as "is the change owner in the attention set", but I don't think there's an operator like that.

The error message can be made clearer, e.g., by pointing to the release notes. But perhaps that wouldn't help for users who obviously cannot trigger it.

Sure, the release notes are more for the Gerrit admin, but it has relevance also for users.

I was wondering *IF* it would make sense to have it configurable on a per-project basis.

The complexity is NOT affordable for certain projects but it IS affordable for others.

At the moment in Gerrit is all or nothing, but for setups like GerritHub.io that are hosting different type of repos it could be NOT the ideal way of having it.

Finally, it looks like I don't understand the reasoning given in the docs. My understanding of a change being "mergeable" is that it can be applied to the current tip of the target branch, whatever it is right now. How does that trigger an O(n^2) complexity per change? Isn't it at most O(n_open_changes * n_diff_hunks_in_change)? Of course if this info goes into the index, it's something like O(n_open_changes) complexity at push time, but that's again just the cost of doing business, right? And even if it O(n^2), well, what is the alternative? As a developer I surely hope I see a red/green thingy indicating to me if I can merge that one change that I'm looking at now. I do not want to find that out only after I've hit the submit button. Is that going to be supported? And if it is, why not show that info in the listing of changes as well?

As mentioned, that is more for the ‘is:mergeable’ queries where you have that information pre-calculated.

The issue was twofold:

1. The value was NOT reliable in terms of accuracy and timing: it was done using the Gerrit’s algorithm for checking if a merge would create conflicts or not, but it isn’t necessarily the case. Countless times I saw the “Merge Conflict” status and then after a rebase it was gone, which means it wasn’t really a conflict, isn’t it?

2. It was creating peaks of CPU utilisations and system load, because if you have 100 open changes towards a repo/branch, all the 100 changes are going to be *fully reindexed* (including Prolog rules execution) every time that one of the changes is modified (rebase, moved, new patch-sets uploaded)

The point 2. creates also a massive delay (on some projects) and creating further lack of trust on how much “up-to-date” is the “Merge Conflict” flag.

On the complexity, if you upload 100 changes (N) and 50 patch-sets (M) per change and merge all of them, you would end up doing:

O(100  * 100 * 50) = O(500k) reindexing operations

Speaking about the dashboards and query results, originally these were paged. Each of the independent queries showed something like "10 or more", and you would only get the full list upon a click (and erven that was still paginated). That's not the case on GerritHub right now, and perhaps a reason why our dashboard [3] takes 10+ seconds to render now.

Let me look into that compare to the v3.3.4 and we’ll adjust the config.

Thanks again for your feedback.

Luca.

With kind regards,
Jan

[1] https://www.gerritcodereview.com/3.4.html#ismergeable-predicate-is-disabled-per-default
[2] https://github.com/Telecominfraproject/oopt-gnpy/commit/89b4da9518237e4a0902ead288a8c6049b4ae1f3
[3] https://review.gerrithub.io/p/Telecominfraproject/oopt-gnpy/+/dashboard/main:main

[Luca Milanesio]

On 20 May 2021, at 17:17, Luca Milanesio <luca.mi...@gmail.com> wrote:

Hi Jan,

Thanks for your feedback, which is always very much appreciated.

Answers inline.

On 20 May 2021, at 14:20, Jan Kundrát <j...@flaska.net> wrote:

Hi,
I read [1] that the is:mergable operator got disabled by default (and we bumped into this as users of GerritHub).
We've had this operator in our stored project dashboard which acted as a landing page. Not a bit deal, we've updated [2] that one, but it's something which raised a support request already.

For us, it was a piece of useful information.

We are re-enabling the is:mergeable on GerritHub.io: by checking the system load, it is something we can still afford at the moment and we are happy to listen to our customers :-)

Current status:

- EU site is enabled again (review-eu.gerrithub.io)

- US site is in-progress (review-am.gerrithub.io)

review.gerrithub.io is going by default to the US site, so it will take a few more minutes.

P.S. GerritHub is multi-site, so all sites are replicated transparently.

We created that dashboard as a tool for users to take a peek at what is brewing in the repo. Our dashboard was split into "what needs work from the original developer", "what needs a reviewer's attention", and "what got done recently". Anything which wouldn't merge went straight into the "needs a developer to finish" bucket. This dashboard of course predates the excellent Attention Sets which I personally just love, and there's a certain overlap in features. Still, the attention set is inherently personalised, and it's an excellent tool for active developers. What we solved with a dashboard is an overview that centers around the full project, not around one particular developer. I think I could get a similar result if there was a query operator such as "is the change owner in the attention set", but I don't think there's an operator like that.

The error message can be made clearer, e.g., by pointing to the release notes. But perhaps that wouldn't help for users who obviously cannot trigger it.

Sure, the release notes are more for the Gerrit admin, but it has relevance also for users.

I was wondering *IF* it would make sense to have it configurable on a per-project basis.

The complexity is NOT affordable for certain projects but it IS affordable for others.

At the moment in Gerrit is all or nothing, but for setups like GerritHub.io that are hosting different type of repos it could be NOT the ideal way of having it.

Raised as:

https://bugs.chromium.org/p/gerrit/issues/detail?id=14569

Luca.

[Luca Milanesio]

On 20 May 2021, at 17:17, Luca Milanesio <luca.mi...@gmail.com> wrote:

Hi Jan,

Thanks for your feedback, which is always very much appreciated.

Answers inline.

On 20 May 2021, at 14:20, Jan Kundrát <j...@flaska.net> wrote:

Hi,
I read [1] that the is:mergable operator got disabled by default (and we bumped into this as users of GerritHub).
We've had this operator in our stored project dashboard which acted as a landing page. Not a bit deal, we've updated [2] that one, but it's something which raised a support request already.

For us, it was a piece of useful information.

We are re-enabling the is:mergeable on GerritHub.io: by checking the system load, it is something we can still afford at the moment and we are happy to listen to our customers :-)

Current status:

- EU site is enabled again (review-eu.gerrithub.io)

- US site is in-progress (review-am.gerrithub.io)

Current status:

- EU site: enabled again

- US site: enabled again

review.gerrithub.io is serving again the ‘is:mergeable’ predicate.

P.S. Only new changes will re-calculate the mergeability, as the full recalculation of all changes would take *days* of computing power anyway :-)

Luca.