opendev/gerrit
Code Issues Proposed changes

Remove asynchronous loading of starred changes

Browse Source 
When the starred changes were stored in the database for performance
reasons it was important to load them asynchronously while a change
query was executed. Now the starred changes are stored in the change
index so that this asynchronous loading is no longer needed.

This removes the starred changes functionality from IdentifiedUser
which was only used by IsStarredByLegacyPredicate.
IsStarredByLegacyPredicate is used to serve the "is:starred" query
when the current change index is old and doesn't contain any starred
changes. In this case we must load the starred changes from the git
backend. This is expensive since all starred changes refs must be
iterated, but implementing some asynchronous loading for this case
seems overkill since IsStarredByLegacyPredicate is only used until the
online reindexing to a newer index version that contains starred
changes is done.

The old implementation of IsStarredByLegacyPredicate was broken since
it tried to query the starred changes from the index which doesn't
contain starred changes when IsStarredByLegacyPredicate is used.

This also fixes the following warning from StarredChangesUtil when the
newest change index version is used from which the deprecated
STARREDBY is removed:

  WARN  com.google.gerrit.server.StarredChangesUtil : Cannot query
  starred changes for account 1000000
  java.lang.IllegalArgumentException: numHits must be > 0; please use
  TotalHitCountCollector if you just need the total hit count

This warning occurred because the asynchronous loading of changes used
the STARREDBY field to load changes from the index, but the index
didn't contain this field anymore.

Since the starred changes functionality is removed from IdentifiedUser
the abstract CurrentUser.getStarredChanges() method is no longer
needed and hence it is removed. This means that all subclasses of
CurrentUser must be adapted. One subclass exists in the replication
plugin and hence this plugin must be updated.

Also, since IdentifiedUser no longer needs StarredChangesUtil, binding
a null provider for this class in tests is no longer needed.

Change-Id: I14a38e9b243b4b182230fe70b023203492a81904
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-05-31 12:50:41 +02:00
parent 96918cce97
commit 3a9014c92e
17 changed files with 83 additions and 219 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@@ -36,7 +36,6 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.FakeRealm;
@@ -169,8 +168,6 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
.toInstance(serverIdent);
bind(GitReferenceUpdated.class)
.toInstance(GitReferenceUpdated.DISABLED);
bind(StarredChangesUtil.class)
.toProvider(Providers.<StarredChangesUtil> of(null));
bind(MetricMaker.class).to(DisabledMetricMaker.class);
bind(ReviewDb.class).toProvider(Providers.<ReviewDb> of(null));
}