Obsolete
Status Update
No update yet.
Comments
da...@gmail.com
[Monorail components: plugins>reviewers]
da...@gmail.com
[Empty comment from Monorail migration]
da...@gmail.com
Thanks for the review.
da...@gmail.com
[Empty comment from Monorail migration]
al...@google.com
When I took a quick glance at the code, I noticed an issue not mentioned above:
For each config entry, the plugin sends a query to the change index to test whether the config entry applies to the change. That's unnecessary and inefficient. It also means that the reviewer-addition functionality is relying on possibly outdated data if the index is out of sync. There's an alternative approach: A query predicate for a change can be directly tested whether it applies to a change (via predicate.asMatchable().match(changeData)). The only prerequisite is that the query operators are limited to those which don't need an index lookup. We should also exclude operators which have expensive post-filtering. The resulting query operators should still be a very large set, which should be sufficient for the use case.
For each config entry, the plugin sends a query to the change index to test whether the config entry applies to the change. That's unnecessary and inefficient. It also means that the reviewer-addition functionality is relying on possibly outdated data if the index is out of sync. There's an alternative approach: A query predicate for a change can be directly tested whether it applies to a change (via predicate.asMatchable().match(changeData)). The only prerequisite is that the query operators are limited to those which don't need an index lookup. We should also exclude operators which have expensive post-filtering. The resulting query operators should still be a very large set, which should be sufficient for the use case.
sv...@axis.com
[Empty comment from Monorail migration]
sv...@axis.com
@Alice
After reflecting on the Comments (on the change) I don't see that *not* querying the index is an improvement. Is this really something we want to do?
It's slower, and adding reviewers is not so critical that we can't gamble on the off-chance that the index is out-dated.
And what effects would an out-dated index mean?
in 1:100 000 changes the wrong reveiwer get's added or a reviewer that should be added isn't. That's an acceptable penalty for performance to me
The absolutely most common case is:
1. change is uploaded
2. Reviewers are added from patchset-created event.
What are the odds that the index gets outdated in a couple of ms?
What am I missing?
After reflecting on the Comments (on the change) I don't see that *not* querying the index is an improvement. Is this really something we want to do?
It's slower, and adding reviewers is not so critical that we can't gamble on the off-chance that the index is out-dated.
And what effects would an out-dated index mean?
in 1:100 000 changes the wrong reveiwer get's added or a reviewer that should be added isn't. That's an acceptable penalty for performance to me
The absolutely most common case is:
1. change is uploaded
2. Reviewers are added from patchset-created event.
What are the odds that the index gets outdated in a couple of ms?
What am I missing?
hi...@google.com
I assume we are talking about this part of the code in terms of index queries:
ChangeQueryBuilder qb = queryBuilder.asUser(user.get());
return !queryProvider
.get()
.noFields()
.query(qb.parse(String.format("change:%s %s", change, filter)))
.isEmpty();
In this setting, you don't actually need the index. You need the index when you have certain criteria and want to know what changes match. In this case, you have just a single change and want to test if the criteria you have match the change. I think in this case we can just construct the predicate tree and call 'match' on the single change we care about.
That spares the call to the index (might be local, but could also be remote which means maybe 100ms of latency per call). The in-memory evaluation should be faster and less prone to failure.
I think we did something similar with checks and it was actually pretty easy.
The problem here specifically is that we are not doing a single index query, but we are doing N in sequence. This can easily become a latency issue.
ChangeQueryBuilder qb = queryBuilder.asUser(user.get());
return !queryProvider
.get()
.noFields()
.query(qb.parse(String.format("change:%s %s", change, filter)))
.isEmpty();
In this setting, you don't actually need the index. You need the index when you have certain criteria and want to know what changes match. In this case, you have just a single change and want to test if the criteria you have match the change. I think in this case we can just construct the predicate tree and call 'match' on the single change we care about.
That spares the call to the index (might be local, but could also be remote which means maybe 100ms of latency per call). The in-memory evaluation should be faster and less prone to failure.
I think we did something similar with checks and it was actually pretty easy.
The problem here specifically is that we are not doing a single index query, but we are doing N in sequence. This can easily become a latency issue.
sv...@axis.com
Can you point to an example from the checks plugin.
This was my stab at not calling the index:
https://gerrit-review.googlesource.com/c/plugins/reviewers/+/265133
Is that what you mean?
Is the set of query operators that doesn't require the index documented somewhere?
This is where the index-call was introduced:
https://gerrit-review.googlesource.com/c/plugins/reviewers/+/160075
It doesn't give any hints as to why it was introduced and which problems were solved.
This was my stab at not calling the index:
Is that what you mean?
Is the set of query operators that doesn't require the index documented somewhere?
This is where the index-call was introduced:
It doesn't give any hints as to why it was introduced and which problems were solved.
da...@gmail.com
> It doesn't give any hints as to why it was introduced and which problems were solved.
As mentioned in the commit message:
"
It's faster then predicate based evaluation and produces consistent results with secondary index used from the UI.
"
The issue was (they were reported to repo-discuss an could be found there), that the gerrit users were trying to formulate and test the filter from the UI (operator1 AND operator2 ) OR (operator3 AND operator4), or some such, just an example, and it was working for them from the UI, but did not work in plugin because it didn't actually used index.
What we could do, is to have both operation modes in place in the plugin and provide a sensible default, but be able to switch to using one or another operation mode.
As mentioned in the commit message:
"
It's faster then predicate based evaluation and produces consistent results with secondary index used from the UI.
"
The issue was (they were reported to repo-discuss an could be found there), that the gerrit users were trying to formulate and test the filter from the UI (operator1 AND operator2 ) OR (operator3 AND operator4), or some such, just an example, and it was working for them from the UI, but did not work in plugin because it didn't actually used index.
What we could do, is to have both operation modes in place in the plugin and provide a sensible default, but be able to switch to using one or another operation mode.
da...@gmail.com
sv...@axis.com
> What we could do, is to have both operation modes in place in the plugin and provide a sensible default, but be able to switch to using one or another operation mode.
I'm all for that.
If you are using a local index it seems to me like there are only benefits to using the index. But there might be a performance hit when you aren't using a local index.
This could be applied on top of the change that refactors all filter-logic to a single class.
https://gerrit-review.googlesource.com/c/plugins/reviewers/+/266171
Still the biggest hurdle seems to me to be how to identify exactly which limitations there are to not using the index, since these still need to be documented.
I'm all for that.
If you are using a local index it seems to me like there are only benefits to using the index. But there might be a performance hit when you aren't using a local index.
This could be applied on top of the change that refactors all filter-logic to a single class.
Still the biggest hurdle seems to me to be how to identify exactly which limitations there are to not using the index, since these still need to be documented.
sv...@axis.com
By limitations I mean "Which query operators will not work when not using the index"
al...@google.com
The CheckerQuery class (https://gerrit.googlesource.com/plugins/checks/+/refs/heads/master/java/com/google/gerrit/plugins/checks/CheckerQuery.java ) of the checks plugin might be worth looking into for this. There's a Javadoc comment starting in line 66 which would be especially interesting. It mentions for instance:
// Predicates that definitely cannot be allowed:
// * Anything where match() needs to query the index, i.e. any full-text fields. This includes
// the default field.
// * Anything where post-filtering is unreasonably expensive.
// * Any predicate over projects, since that may conflict with the projects field.
// Predicates that definitely cannot be allowed:
// * Anything where match() needs to query the index, i.e. any full-text fields. This includes
// the default field.
// * Anything where post-filtering is unreasonably expensive.
// * Any predicate over projects, since that may conflict with the projects field.
sv...@axis.com
Thanks Alice. That set (ALLOWED_OPERATORS) is a good starting point, too bad we have to copy it to the reviewers plugin.
Predicates being able to tell you if they need the index or not would be a great way to not having to worry about arbitrary sets copied to various plugins but I think I'll start here.
Predicates being able to tell you if they need the index or not would be a great way to not having to worry about arbitrary sets copied to various plugins but I think I'll start here.
sv...@axis.com
I have updated the change with documentation and validation of the supported operators as copied from checks plugin.
https://gerrit-review.googlesource.com/c/plugins/reviewers/+/265133
Description
As part of the assessment if we should do this, the ESC looked into the plugin from different dimensions:
1) UI/UX (Ben)
2) Usage (Luca)
3) Applicability (Alice)
4) Code quality (Patrick).
This bug tracks the outcome of looking over the code of the plugin and identify gaps that should be fixed before it becomes a core plugin.
The plugin doesn't have a lot of backend code (good). The style and usage of APIs is in-line with what we would expect in Gerrit core. There are integration tests (good) that test configuration and usage of the plugin (calling it via extension points in core).
1) Add JavaDoc to classes. One-liners are OK, but intent should be explained, so that casual readers understand how it fits together.
2) ReviewersIT uses Thread.sleep to wait out calls from Gerrit core / processing. Core is calling plugins synchronously, so it should be possible to get the test written without thread waiting. The plugin internally uses a WorkQueue, which makes processing async, but we can bind that to a directExecutor() in the test to make it sync and remove the Thread waiting.
3) The "test" prefixes should be removed from methods to match style in core (accountGroupResolve instead of testAccountGroupResolve)
4) Add tests for special case changes (private, WiP)
5) Add tests for change visibility (configured to add a user as reviewer that can't see the change)