Minimize use of ReviewDb when not needed
When all changes have been migrated to NoteDb, do not open ReviewDb
anymore. Opening ReviewDb when not needed would only cause more
contention on the DB Pool and cause problems to the active threads.
The only point where we need to "mimic" a minimum of functionality
is when a ChangeNote is retrieved: just return a null element so
that Gerrit can get the entire data from the Git repository, avoiding
stale indexing.
Bug: Issue 9302
Change-Id: I8b0ba81ef16c546852c0d93f27bb8abb46c034f9
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java
index f7ea962..3a0e686 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandler.java
@@ -18,6 +18,7 @@
import com.ericsson.gerrit.plugins.highavailability.Configuration.Index;
import com.ericsson.gerrit.plugins.highavailability.index.ChangeChecker;
import com.ericsson.gerrit.plugins.highavailability.index.ChangeCheckerImpl;
+import com.ericsson.gerrit.plugins.highavailability.index.ChangeDb;
import com.ericsson.gerrit.plugins.highavailability.index.ForwardedIndexExecutor;
import com.google.common.base.Splitter;
import com.google.gerrit.reviewdb.client.Change;
@@ -29,7 +30,6 @@
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
-import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -46,7 +46,7 @@
@Singleton
public class ForwardedIndexChangeHandler extends ForwardedIndexingHandler<String> {
private final ChangeIndexer indexer;
- private final SchemaFactory<ReviewDb> schemaFactory;
+ private final ChangeDb changeDb;
private final ScheduledExecutorService indexExecutor;
private final OneOffRequestContext oneOffCtx;
private final int retryInterval;
@@ -56,7 +56,7 @@
@Inject
ForwardedIndexChangeHandler(
ChangeIndexer indexer,
- SchemaFactory<ReviewDb> schemaFactory,
+ ChangeDb changeDb,
ChangeFinder changeFinder,
Configuration config,
@ForwardedIndexExecutor ScheduledExecutorService indexExecutor,
@@ -64,7 +64,7 @@
ChangeCheckerImpl.Factory changeCheckerFactory) {
super(config.index());
this.indexer = indexer;
- this.schemaFactory = schemaFactory;
+ this.changeDb = changeDb;
this.indexExecutor = indexExecutor;
this.oneOffCtx = oneOffCtx;
this.changeCheckerFactory = changeCheckerFactory;
@@ -82,13 +82,12 @@
private void doIndex(String id, Optional<IndexEvent> indexEvent, int retryCount)
throws IOException, OrmException {
- try (ReviewDb db = schemaFactory.open()) {
+ try {
ChangeChecker checker = changeCheckerFactory.create(id);
Optional<ChangeNotes> changeNotes = checker.getChangeNotes();
if (changeNotes.isPresent()) {
ChangeNotes notes = changeNotes.get();
- notes.reload();
- indexer.index(db, notes.getChange());
+ reindex(notes);
if (checker.isChangeUpToDate(indexEvent)) {
if (retryCount > 0) {
@@ -122,6 +121,13 @@
}
}
+ private void reindex(ChangeNotes notes) throws IOException, OrmException {
+ try (ReviewDb db = changeDb.open()) {
+ notes.reload();
+ indexer.index(db, notes.getChange());
+ }
+ }
+
private void rescheduleIndex(String id, Optional<IndexEvent> indexEvent, int retryCount) {
if (retryCount > maxTries) {
log.error(
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
index 7b662dd..88aa5ca 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
@@ -25,7 +25,6 @@
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
-import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
@@ -41,7 +40,7 @@
private static final Logger log = LoggerFactory.getLogger(ChangeCheckerImpl.class);
private final GitRepositoryManager gitRepoMgr;
private final CommentsUtil commentsUtil;
- private final SchemaFactory<ReviewDb> schemaFactory;
+ private final ChangeDb changeDb;
private final OneOffRequestContext oneOffReqCtx;
private final String changeId;
private final ChangeFinder changeFinder;
@@ -56,14 +55,14 @@
public ChangeCheckerImpl(
GitRepositoryManager gitRepoMgr,
CommentsUtil commentsUtil,
- SchemaFactory<ReviewDb> schemaFactory,
+ ChangeDb changeDb,
ChangeFinder changeFinder,
OneOffRequestContext oneOffReqCtx,
@Assisted String changeId) {
this.changeFinder = changeFinder;
this.gitRepoMgr = gitRepoMgr;
this.commentsUtil = commentsUtil;
- this.schemaFactory = schemaFactory;
+ this.changeDb = changeDb;
this.oneOffReqCtx = oneOffReqCtx;
this.changeId = changeId;
}
@@ -80,21 +79,6 @@
});
}
- private String getBranchTargetSha() {
- try (Repository repo = gitRepoMgr.openRepository(changeNotes.get().getProjectName())) {
- String refName = changeNotes.get().getChange().getDest().get();
- Ref ref = repo.findRef(refName);
- if (ref == null) {
- log.warn("Unable to find target ref {} for change {}", refName, changeId);
- return null;
- }
- return ref.getTarget().getObjectId().getName();
- } catch (IOException e) {
- log.warn("Unable to resolve target branch SHA for change {}", changeId, e);
- return null;
- }
- }
-
@Override
public Optional<ChangeNotes> getChangeNotes() throws OrmException {
try (ManualRequestContext ctx = oneOffReqCtx.open()) {
@@ -113,13 +97,16 @@
return false;
}
- String targetSha = getBranchTargetSha();
+ if (indexEvent.isPresent() && indexEvent.get().targetSha == null) {
+ return indexEvent.map(e -> (computedChangeTs.get() >= e.eventCreatedOn)).orElse(true);
+ }
+
return indexEvent
.map(
e ->
(computedChangeTs.get() > e.eventCreatedOn)
|| (computedChangeTs.get() == e.eventCreatedOn)
- && (Objects.equals(targetSha, e.targetSha)))
+ && (Objects.equals(getBranchTargetSha(), e.targetSha)))
.orElse(true);
}
@@ -146,8 +133,23 @@
}
}
+ private String getBranchTargetSha() {
+ try (Repository repo = gitRepoMgr.openRepository(changeNotes.get().getProjectName())) {
+ String refName = changeNotes.get().getChange().getDest().get();
+ Ref ref = repo.exactRef(refName);
+ if (ref == null) {
+ log.warn("Unable to find target ref {} for change {}", refName, changeId);
+ return null;
+ }
+ return ref.getTarget().getObjectId().getName();
+ } catch (IOException e) {
+ log.warn("Unable to resolve target branch SHA for change {}", changeId, e);
+ return null;
+ }
+ }
+
private Optional<Long> computeLastChangeTs() throws OrmException {
- try (ReviewDb db = schemaFactory.open()) {
+ try (ReviewDb db = changeDb.open()) {
return getChangeNotes().map(notes -> getTsFromChangeAndDraftComments(db, notes));
}
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeDb.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeDb.java
new file mode 100644
index 0000000..bef5363
--- /dev/null
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeDb.java
@@ -0,0 +1,38 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.ericsson.gerrit.plugins.highavailability.index;
+
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.SchemaFactory;
+import com.google.inject.Inject;
+
+public class ChangeDb {
+ private static final DisabledReviewDb disabledReviewDb = new DisabledReviewDb();
+
+ private final NotesMigration migration;
+ private final SchemaFactory<ReviewDb> schemaFactory;
+
+ @Inject
+ public ChangeDb(NotesMigration migration, SchemaFactory<ReviewDb> schemaFactory) {
+ this.migration = migration;
+ this.schemaFactory = schemaFactory;
+ }
+
+ public ReviewDb open() throws OrmException {
+ return migration.readChanges() ? disabledReviewDb : schemaFactory.open();
+ }
+}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/DisabledReviewDb.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/DisabledReviewDb.java
new file mode 100644
index 0000000..192ee8c
--- /dev/null
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/DisabledReviewDb.java
@@ -0,0 +1,244 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.ericsson.gerrit.plugins.highavailability.index;
+
+import com.google.common.util.concurrent.CheckedFuture;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Change.Id;
+import com.google.gerrit.reviewdb.server.AccountGroupAccess;
+import com.google.gerrit.reviewdb.server.AccountGroupByIdAccess;
+import com.google.gerrit.reviewdb.server.AccountGroupByIdAudAccess;
+import com.google.gerrit.reviewdb.server.AccountGroupMemberAccess;
+import com.google.gerrit.reviewdb.server.AccountGroupMemberAuditAccess;
+import com.google.gerrit.reviewdb.server.AccountGroupNameAccess;
+import com.google.gerrit.reviewdb.server.ChangeAccess;
+import com.google.gerrit.reviewdb.server.ChangeMessageAccess;
+import com.google.gerrit.reviewdb.server.PatchLineCommentAccess;
+import com.google.gerrit.reviewdb.server.PatchSetAccess;
+import com.google.gerrit.reviewdb.server.PatchSetApprovalAccess;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.SchemaVersionAccess;
+import com.google.gerrit.reviewdb.server.SystemConfigAccess;
+import com.google.gwtorm.server.Access;
+import com.google.gwtorm.server.AtomicUpdate;
+import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.ResultSet;
+import com.google.gwtorm.server.StatementExecutor;
+import java.util.Map;
+
+/** ReviewDb that is disabled. */
+@SuppressWarnings("deprecation")
+public class DisabledReviewDb implements ReviewDb {
+ public static class Disabled extends RuntimeException {
+ private static final long serialVersionUID = 1L;
+
+ private Disabled() {
+ super("ReviewDb is disabled for changes");
+ }
+ }
+
+ public static class DisabledChangeAccess implements ChangeAccess {
+
+ @Override
+ public String getRelationName() {
+ throw new Disabled();
+ }
+
+ @Override
+ public int getRelationID() {
+ throw new Disabled();
+ }
+
+ @Override
+ public ResultSet<Change> iterateAllEntities() throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public Id primaryKey(Change entity) {
+ throw new Disabled();
+ }
+
+ @Override
+ public Map<Id, Change> toMap(Iterable<Change> c) {
+ throw new Disabled();
+ }
+
+ @Override
+ public CheckedFuture<Change, OrmException> getAsync(Id key) {
+ throw new Disabled();
+ }
+
+ @Override
+ public ResultSet<Change> get(Iterable<Id> keys) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void insert(Iterable<Change> instances) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void update(Iterable<Change> instances) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void upsert(Iterable<Change> instances) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void deleteKeys(Iterable<Id> keys) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void delete(Iterable<Change> instances) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public void beginTransaction(Id key) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public Change atomicUpdate(Id key, AtomicUpdate<Change> update) throws OrmException {
+ throw new Disabled();
+ }
+
+ @Override
+ public Change get(Id id) throws OrmException {
+ return null;
+ }
+
+ @Override
+ public ResultSet<Change> all() throws OrmException {
+ return null;
+ }
+ }
+
+ @Override
+ public void close() {
+ // Do nothing.
+ }
+
+ @Override
+ public void commit() {
+ throw new Disabled();
+ }
+
+ @Override
+ public void rollback() {
+ throw new Disabled();
+ }
+
+ @Override
+ public void updateSchema(StatementExecutor e) {
+ throw new Disabled();
+ }
+
+ @Override
+ public void pruneSchema(StatementExecutor e) {
+ throw new Disabled();
+ }
+
+ @Override
+ public Access<?, ?>[] allRelations() {
+ throw new Disabled();
+ }
+
+ @Override
+ public SchemaVersionAccess schemaVersion() {
+ throw new Disabled();
+ }
+
+ @Override
+ public SystemConfigAccess systemConfig() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupAccess accountGroups() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupNameAccess accountGroupNames() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupMemberAccess accountGroupMembers() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupMemberAuditAccess accountGroupMembersAudit() {
+ throw new Disabled();
+ }
+
+ @Override
+ public ChangeAccess changes() {
+ return new DisabledChangeAccess();
+ }
+
+ @Override
+ public PatchSetApprovalAccess patchSetApprovals() {
+ throw new Disabled();
+ }
+
+ @Override
+ public ChangeMessageAccess changeMessages() {
+ throw new Disabled();
+ }
+
+ @Override
+ public PatchSetAccess patchSets() {
+ throw new Disabled();
+ }
+
+ @Override
+ public PatchLineCommentAccess patchComments() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupByIdAccess accountGroupById() {
+ throw new Disabled();
+ }
+
+ @Override
+ public AccountGroupByIdAudAccess accountGroupByIdAud() {
+ throw new Disabled();
+ }
+
+ @Override
+ public int nextAccountId() {
+ throw new Disabled();
+ }
+
+ @Override
+ public int nextAccountGroupId() {
+ throw new Disabled();
+ }
+
+ @Override
+ public int nextChangeId() {
+ throw new Disabled();
+ }
+}
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java
index 9ec0d3c..35aee31 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedIndexChangeHandlerTest.java
@@ -27,6 +27,7 @@
import com.ericsson.gerrit.plugins.highavailability.forwarder.ForwardedIndexingHandler.Operation;
import com.ericsson.gerrit.plugins.highavailability.index.ChangeChecker;
import com.ericsson.gerrit.plugins.highavailability.index.ChangeCheckerImpl;
+import com.ericsson.gerrit.plugins.highavailability.index.ChangeDb;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -34,10 +35,8 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
-import com.google.gwtorm.server.SchemaFactory;
import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.ScheduledExecutorService;
@@ -67,7 +66,7 @@
@Rule public ExpectedException exception = ExpectedException.none();
@Mock private ChangeIndexer indexerMock;
- @Mock private SchemaFactory<ReviewDb> schemaFactoryMock;
+ @Mock private ChangeDb changeDbMock;
@Mock private ReviewDb dbMock;
@Mock private ChangeNotes changeNotes;
@Mock private Configuration configMock;
@@ -85,7 +84,7 @@
@Before
public void setUp() throws Exception {
- when(schemaFactoryMock.open()).thenReturn(dbMock);
+ when(changeDbMock.open()).thenReturn(dbMock);
id = new Change.Id(TEST_CHANGE_NUMBER);
change = new Change(null, id, null, null, TimeUtil.nowTs());
when(changeNotes.getChange()).thenReturn(change);
@@ -95,7 +94,7 @@
handler =
new ForwardedIndexChangeHandler(
indexerMock,
- schemaFactoryMock,
+ changeDbMock,
changeFinderMock,
configMock,
indexExecutorMock,
@@ -138,21 +137,6 @@
}
@Test
- public void indexerThrowsNoSuchChangeExceptionTryingToPostChange() throws Exception {
- doThrow(new NoSuchChangeException(id)).when(schemaFactoryMock).open();
- handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.empty());
- verify(indexerMock, times(1)).delete(id);
- }
-
- @Test
- public void indexerThrowsNestedNoSuchChangeExceptionTryingToPostChange() throws Exception {
- OrmException e = new OrmException("test", new NoSuchChangeException(id));
- doThrow(e).when(schemaFactoryMock).open();
- handler.index(TEST_CHANGE_ID, Operation.INDEX, Optional.empty());
- verify(indexerMock, times(1)).delete(id);
- }
-
- @Test
public void indexerThrowsIOExceptionTryingToIndexChange() throws Exception {
setupChangeAccessRelatedMocks(
CHANGE_EXISTS, DO_NOT_THROW_ORM_EXCEPTION, THROW_IO_EXCEPTION, CHANGE_UP_TO_DATE);
@@ -222,17 +206,18 @@
boolean changeExists, boolean ormException, boolean ioException, boolean changeIsUpToDate)
throws OrmException, IOException {
if (ormException) {
- doThrow(new OrmException("")).when(schemaFactoryMock).open();
+ doThrow(new OrmException("")).when(changeDbMock).open();
} else {
- when(schemaFactoryMock.open()).thenReturn(dbMock);
- if (changeExists) {
- when(changeCheckerFactoryMock.create(TEST_CHANGE_ID)).thenReturn(changeCheckerPresentMock);
- when(changeCheckerPresentMock.getChangeNotes()).thenReturn(Optional.of(changeNotes));
- if (ioException) {
- doThrow(new IOException("io-error"))
- .when(indexerMock)
- .index(any(ReviewDb.class), any(Change.class));
- }
+ when(changeDbMock.open()).thenReturn(dbMock);
+ }
+
+ if (changeExists) {
+ when(changeCheckerFactoryMock.create(TEST_CHANGE_ID)).thenReturn(changeCheckerPresentMock);
+ when(changeCheckerPresentMock.getChangeNotes()).thenReturn(Optional.of(changeNotes));
+ if (ioException) {
+ doThrow(new IOException("io-error"))
+ .when(indexerMock)
+ .index(any(ReviewDb.class), any(Change.class));
}
}
