From 46bd79f917f39aad98fc25324d710b2665446a58 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Tue, 13 Jul 2021 14:05:52 +0200 Subject: RefAdvertisementIT: Don't call reindex that would fail silently receivePackOmitsMissingObject wants to test a scenario where we have a change ref, but the patch set ref / commit was deleted or is otherwise gone. It did that by adding a new patch set pointing to a non-existent SHA1 and triggered a reindex of that change. However, that reindex fails silently because we submit the reindex task to another executor and provide no feedback to the caller. This commit modifies the test and just deletes the patch set ref which makes for the same scenario but without the reindex trouble. This change is part of a series that allows for the fake change index to run in integration tests. The fake index throws an exception in case reindexing fails, which made this test fail. Release-Notes: skip Change-Id: Icf79795a343a6abca28c6504b279eb0a5c84fad2 --- .../gerrit/acceptance/git/RefAdvertisementIT.java | 33 ++-------------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 1083377b80..a96fbcfa7e 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -39,7 +39,6 @@ import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.Permission; import com.google.gerrit.entities.AccountGroup; -import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Patch; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; @@ -68,7 +67,6 @@ import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; @@ -1061,37 +1059,10 @@ public class RefAdvertisementIT extends AbstractDaemonTest { @Test public void receivePackOmitsMissingObject() throws Exception { - String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; try (Repository repo = repoManager.openRepository(project); TestRepository tr = new TestRepository<>(repo)) { - String subject = "Subject for missing commit"; - Change c = new Change(cd3.change()); - PatchSet.Id psId = PatchSet.id(cd3.getId(), 2); - c.setCurrentPatchSet(psId, subject, c.getOriginalSubject()); - - PersonIdent committer = serverIdent.get(); - PersonIdent author = - noteUtil.newIdent(getAccount(admin.id()).id(), committer.getWhen(), committer); - tr.branch(RefNames.changeMetaRef(cd3.getId())) - .commit() - .author(author) - .committer(committer) - .message( - "Update patch set " - + psId.get() - + "\n" - + "\n" - + "Patch-set: " - + psId.get() - + "\n" - + "Commit: " - + rev - + "\n" - + "Subject: " - + subject - + "\n") - .create(); - indexer.index(c.getProject(), c.getId()); + PatchSet.Id psId = PatchSet.id(cd3.getId(), 1); + tr.delete(psId.toRefName()); } assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd4, 1)); -- cgit v1.2.3 From 53291cf2e490df210733c33a4ee95b81d69cba41 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Tue, 13 Jul 2021 14:10:24 +0200 Subject: ConsistencyCheckerIT: Delete calls to index broken changes that fail silently These calls used to fail silently and are of no further use. This change is part of a series that allows for the fake change index to run in integration tests. The fake index throws an exception in case reindexing fails, which made this test fail. Release-Notes: skip Change-Id: I9c9d3f7613bca057bf0963442077bb6c0b3dab51 --- .../google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 069387cb90..a0ac315c74 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -257,7 +257,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { + "Groups: " + rev + "\n"); - indexer.index(c.getProject(), c.getId()); ChangeNotes notes = changeNotesFactory.create(c.getProject(), c.getId()); FixInput fix = new FixInput(); @@ -817,8 +816,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { + "Subject: " + subject + "\n"); - indexer.index(c.getProject(), c.getId()); - return ps; } -- cgit v1.2.3 From 081a9914a3b75e33d94f8d176882c044879895b0 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 30 Nov 2023 23:23:35 +0000 Subject: Make the indexing operation fail upon StorageException(s) Change 270450 caused the blanking of the Lucene document upon reindexing if any field caused a StorageException. Whilst the overall intention was good, the implementation caused the Lucene index replace operation to continue with a Document without any fields instead of making the whole operation fail. StorageExceptions are thrown when the underlying storage, being a filesystem or anything else, returns some errors. Whether the failure is permanent or temporary (e.g. concurrent GCs, repacking or pruning may cause sporadic StorageException(s)) returning a blank Lucene document was incorrect because, instead of failing the operation, it was causing the change entry to be completely removed from the index. Let the StorageException fail the indexing operation, so that existing entries may continue to exist, allowing the caller to retry the operation. The previous implementation, returning an empty Document, did not allow any retry, because once the change entry was removed from the index, it could not be discovered and reindexed anymore for example by a `gerrit index changes `. Tested manually, applying a randomic StorageException thrown during the fetching of the ChangeField extensions: public static Set getExtensions(ChangeData cd) { if (new Random().nextBoolean()) { throw new StorageException("Simulated storage exception"); } return extensions(cd).collect(toSet()); } Before this change, every time one change indexing throws a StorageException, it disappears from the index. Eventually, all changes will be eliminated and only an off-line reindex or the reindex of all changes in the project would allow to recover them. After this change, some of the indexing operations are successful and other fails. However, retrying the reindexing operation would allow to reindex all of them. Even if the above test case looks strange at first sight, it simulates a real-life scenario where a low percentage of indexing operation (0.1%) may fail because of sporadic StorageExceptions. Before this change, some index entries were missing on a daily basis (5 to 10 changes per day), whilst after this change all indexing operation can be retried and do not result in any indexing entry loss. Bug: Issue 314113030 Release-Notes: Fail the change reindex operation upon StorageException(s) Change-Id: Ia121f47f7a68c290849a22dea657804743a26b0d --- java/com/google/gerrit/index/Schema.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/index/Schema.java b/java/com/google/gerrit/index/Schema.java index 3aa9de0614..ec14a154f4 100644 --- a/java/com/google/gerrit/index/Schema.java +++ b/java/com/google/gerrit/index/Schema.java @@ -207,21 +207,19 @@ public class Schema { /** * Build all fields in the schema from an input object. * - *

Null values are omitted, as are fields which cause errors, which are logged. + *

Null values are omitted, as are fields which cause errors, which are logged. If any of the + * fields cause a StorageException, the whole operation fails and the exception is propagated to + * the caller. * * @param obj input object. * @param skipFields set of field names to skip when indexing the document * @return all non-null field values from the object. */ public final Iterable> buildFields(T obj, ImmutableSet skipFields) { - try { - return fields.values().stream() - .map(f -> fieldValues(obj, f, skipFields)) - .filter(Objects::nonNull) - .collect(toImmutableList()); - } catch (StorageException e) { - return ImmutableList.of(); - } + return fields.values().stream() + .map(f -> fieldValues(obj, f, skipFields)) + .filter(Objects::nonNull) + .collect(toImmutableList()); } @Override -- cgit v1.2.3