Merge branch 'stable-3.5' into stable-3.6 * stable-3.5: PaginatingSource: Stop matching changes after desired limit is reached Don't retry new change creation on lock failure pgm-init.txt: remove reference to Review DB from --batch option Don't fail metric computation due to duplicated metric names DefaultMemoryCacheFactoryTest: Fix flaky test Update JGit to 5166ded098 Update JGit to 0687c73a12 Release-Notes: skip Change-Id: Ia4aafac2df67c70da9684f1ac98aa583fe6b8cc5
diff --git a/Documentation/pgm-init.txt b/Documentation/pgm-init.txt index 3c9e3fc..4b346fe 100644 --- a/Documentation/pgm-init.txt +++ b/Documentation/pgm-init.txt
@@ -38,11 +38,6 @@ install, reasonable configuration defaults are chosen based on the whims of the Gerrit developers. On upgrades, the existing settings in `gerrit.config` are respected. -+ -If during a schema migration unused objects (e.g. tables, columns) -are detected, they are *not* automatically dropped; a list of SQL -statements to drop these objects is provided. To drop the unused -objects these SQL statements must be executed manually. --delete-caches:: Force deletion of all persistent cache files. Note that
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java index fd3a218..b05c8f4 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -87,6 +87,9 @@ r.add(data); } pageResultSize++; + if (r.size() > limit) { + break; + } } nextStart += pageResultSize; searchAfter = next.searchAfter();
diff --git a/java/com/google/gerrit/metrics/dropwizard/BUILD b/java/com/google/gerrit/metrics/dropwizard/BUILD index 4b3859f..dbb8f5e 100644 --- a/java/com/google/gerrit/metrics/dropwizard/BUILD +++ b/java/com/google/gerrit/metrics/dropwizard/BUILD
@@ -12,6 +12,7 @@ "//lib:args4j", "//lib:guava", "//lib/dropwizard:dropwizard-core", + "//lib/flogger:api", "//lib/guice", ], )
diff --git a/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java b/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java index b3860f7..da9ec70 100644 --- a/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java +++ b/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java
@@ -110,7 +110,7 @@ } } - private String submetric(Object key) { + String submetric(Object key) { return DropWizardMetricMaker.name(ordering, name, name(key)); }
diff --git a/java/com/google/gerrit/metrics/dropwizard/CallbackMetricImpl1.java b/java/com/google/gerrit/metrics/dropwizard/CallbackMetricImpl1.java index d718035..bd3caf9 100644 --- a/java/com/google/gerrit/metrics/dropwizard/CallbackMetricImpl1.java +++ b/java/com/google/gerrit/metrics/dropwizard/CallbackMetricImpl1.java
@@ -15,13 +15,17 @@ package com.google.gerrit.metrics.dropwizard; import com.codahale.metrics.MetricRegistry; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.metrics.CallbackMetric1; import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Field; +import java.util.concurrent.TimeUnit; import java.util.function.Function; /** Optimized version of {@link BucketedCallback} for single dimension. */ class CallbackMetricImpl1<F1, V> extends BucketedCallback<V> { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + CallbackMetricImpl1( DropWizardMetricMaker metrics, MetricRegistry registry, @@ -44,9 +48,14 @@ @Override public void set(F1 field1, V value) { - BucketedCallback<V>.ValueGauge cell = getOrCreate(field1); - cell.value = value; - cell.set = true; + try { + BucketedCallback<V>.ValueGauge cell = getOrCreate(field1); + cell.value = value; + cell.set = true; + } catch (IllegalArgumentException e) { + logger.atWarning().withCause(e).atMostEvery(1, TimeUnit.HOURS).log( + "Unable to register duplicate metric: %s", submetric(field1)); + } } @Override
diff --git a/java/com/google/gerrit/server/git/MultiProgressMonitor.java b/java/com/google/gerrit/server/git/MultiProgressMonitor.java index 290e1e7..b88985d 100644 --- a/java/com/google/gerrit/server/git/MultiProgressMonitor.java +++ b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
@@ -168,6 +168,11 @@ public String getTotalDisplay(int total) { return String.valueOf(total); } + + @Override + public void showDuration(boolean enabled) { + // not implemented + } } /** Handle for a sub-task whose total work can be updated while the task is in progress. */
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 1cebf5f..71606dc 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1024,111 +1024,29 @@ return; } try { - retryHelper - .changeUpdate( - "insertChangesAndPatchSets", - updateFactory -> { - try (BatchUpdate bu = - updateFactory.create( - project.getNameKey(), user.materializedCopy(), TimeUtil.now()); - ObjectInserter ins = repo.newObjectInserter(); - ObjectReader reader = ins.newReader(); - RevWalk rw = new RevWalk(reader)) { - bu.setRepository(repo, rw, ins); - bu.setRefLogMessage("push"); - if (magicBranch != null) { - bu.setNotify(magicBranch.getNotifyForNewChange()); - } - - logger.atFine().log("Adding %d replace requests", newChanges.size()); - for (ReplaceRequest replace : replaceByChange.values()) { - replace.addOps(bu, replaceProgress); - if (magicBranch != null) { - bu.setNotifyHandling( - replace.ontoChange, magicBranch.getNotifyHandling(replace.notes)); - if (magicBranch.shouldPublishComments()) { - bu.addOp( - replace.notes.getChangeId(), - publishCommentsOp.create(replace.psId, project.getNameKey())); - Optional<ChangeNotes> changeNotes = - getChangeNotes(replace.notes.getChangeId()); - if (!changeNotes.isPresent()) { - // If not present, no need to update attention set here since this is a - // new change. - continue; - } - List<HumanComment> drafts = - commentsUtil.draftByChangeAuthor( - changeNotes.get(), user.getAccountId()); - if (drafts.isEmpty()) { - // If no comments, attention set shouldn't update since the user didn't - // reply. - continue; - } - replyAttentionSetUpdates.processAutomaticAttentionSetRulesOnReply( - bu, - changeNotes.get(), - isReadyForReview(changeNotes.get()), - user, - drafts); - } - } - } - - logger.atFine().log("Adding %d create requests", newChanges.size()); - for (CreateRequest create : newChanges) { - create.addOps(bu); - } - - logger.atFine().log("Adding %d group update requests", newChanges.size()); - updateGroups.forEach(r -> r.addOps(bu)); - - logger.atFine().log("Executing batch"); - try { - bu.execute(); - } catch (UpdateException e) { - throw asRestApiException(e); - } - - replaceByChange.values().stream() - .forEach( - req -> - result.addChange( - ReceiveCommitsResult.ChangeStatus.REPLACED, req.ontoChange)); - newChanges.stream() - .forEach( - req -> - result.addChange( - ReceiveCommitsResult.ChangeStatus.CREATED, req.changeId)); - - if (magicBranchCmd != null) { - magicBranchCmd.setResult(OK); - } - for (ReplaceRequest replace : replaceByChange.values()) { - String rejectMessage = replace.getRejectMessage(); - if (rejectMessage == null) { - if (replace.inputCommand.getResult() == NOT_ATTEMPTED) { - // Not necessarily the magic branch, so need to set OK on the original - // value. - replace.inputCommand.setResult(OK); - } - } else { - logger.atFine().log("Rejecting due to message from ReplaceOp"); - reject(replace.inputCommand, rejectMessage); - } - } - } - return null; - }) - .defaultTimeoutMultiplier(5) - .call(); + if (!newChanges.isEmpty()) { + // TODO: Retry lock failures on new change insertions. The retry will + // likely have to move to a higher layer to be able to achieve that + // due to state that needs to be reset with each retry attempt. + insertChangesAndPatchSets(magicBranchCmd, newChanges, replaceProgress); + } else { + retryHelper + .changeUpdate( + "insertPatchSets", + updateFactory -> { + insertChangesAndPatchSets(magicBranchCmd, newChanges, replaceProgress); + return null; + }) + .defaultTimeoutMultiplier(5) + .call(); + } } catch (ResourceConflictException e) { addError(e.getMessage()); reject(magicBranchCmd, "conflict"); } catch (BadRequestException | UnprocessableEntityException | AuthException e) { logger.atFine().withCause(e).log("Rejecting due to client error"); reject(magicBranchCmd, e.getMessage()); - } catch (RestApiException | UpdateException e) { + } catch (RestApiException | IOException | UpdateException e) { throw new StorageException("Can't insert change/patch set for " + project.getName(), e); } @@ -1151,6 +1069,90 @@ } } + private void insertChangesAndPatchSets( + ReceiveCommand magicBranchCmd, List<CreateRequest> newChanges, Task replaceProgress) + throws RestApiException, IOException { + try (BatchUpdate bu = + batchUpdateFactory.create( + project.getNameKey(), user.materializedCopy(), TimeUtil.now()); + ObjectInserter ins = repo.newObjectInserter(); + ObjectReader reader = ins.newReader(); + RevWalk rw = new RevWalk(reader)) { + bu.setRepository(repo, rw, ins); + bu.setRefLogMessage("push"); + if (magicBranch != null) { + bu.setNotify(magicBranch.getNotifyForNewChange()); + } + + logger.atFine().log("Adding %d replace requests", newChanges.size()); + for (ReplaceRequest replace : replaceByChange.values()) { + replace.addOps(bu, replaceProgress); + if (magicBranch != null) { + bu.setNotifyHandling(replace.ontoChange, magicBranch.getNotifyHandling(replace.notes)); + if (magicBranch.shouldPublishComments()) { + bu.addOp( + replace.notes.getChangeId(), + publishCommentsOp.create(replace.psId, project.getNameKey())); + Optional<ChangeNotes> changeNotes = getChangeNotes(replace.notes.getChangeId()); + if (!changeNotes.isPresent()) { + // If not present, no need to update attention set here since this is a + // new change. + continue; + } + List<HumanComment> drafts = + commentsUtil.draftByChangeAuthor(changeNotes.get(), user.getAccountId()); + if (drafts.isEmpty()) { + // If no comments, attention set shouldn't update since the user didn't + // reply. + continue; + } + replyAttentionSetUpdates.processAutomaticAttentionSetRulesOnReply( + bu, changeNotes.get(), isReadyForReview(changeNotes.get()), user, drafts); + } + } + } + + logger.atFine().log("Adding %d create requests", newChanges.size()); + for (CreateRequest create : newChanges) { + create.addOps(bu); + } + + logger.atFine().log("Adding %d group update requests", newChanges.size()); + updateGroups.forEach(r -> r.addOps(bu)); + + logger.atFine().log("Executing batch"); + try { + bu.execute(); + } catch (UpdateException e) { + throw asRestApiException(e); + } + + replaceByChange.values().stream() + .forEach( + req -> result.addChange(ReceiveCommitsResult.ChangeStatus.REPLACED, req.ontoChange)); + newChanges.stream() + .forEach( + req -> result.addChange(ReceiveCommitsResult.ChangeStatus.CREATED, req.changeId)); + + if (magicBranchCmd != null) { + magicBranchCmd.setResult(OK); + } + for (ReplaceRequest replace : replaceByChange.values()) { + String rejectMessage = replace.getRejectMessage(); + if (rejectMessage == null) { + if (replace.inputCommand.getResult() == NOT_ATTEMPTED) { + // Not necessarily the magic branch, so need to set OK on the original + // value. + replace.inputCommand.setResult(OK); + } + } else { + logger.atFine().log("Rejecting due to message from ReplaceOp"); + reject(replace.inputCommand, rejectMessage); + } + } + } + } + private boolean isReadyForReview(ChangeNotes changeNotes) { return (!changeNotes.getChange().isWorkInProgress() && !magicBranch.workInProgress) || magicBranch.ready;
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java index 2518391..0771afa 100644 --- a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java +++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -145,12 +145,11 @@ cacheFactory.build(newCacheDef(1), newCacheLoader(identity())); cache.put(TEST_CACHE_KEY, TEST_CACHE_VALUE); - cache.invalidate(TEST_CACHE_KEY); assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isFalse(); + cache.invalidate(TEST_CACHE_KEY); evictionReceived.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS); - assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isTrue(); assertThat(forwardingRemovalListener.removalThreadName(TEST_CACHE_KEY)) .startsWith(threadPoolPrefix);
diff --git a/modules/jgit b/modules/jgit index 176f17d..5166ded 160000 --- a/modules/jgit +++ b/modules/jgit
@@ -1 +1 @@ -Subproject commit 176f17d05ec154ce455ab2bde7429017d43d67fb +Subproject commit 5166ded0986df7a99bbc9ae6bc057a27a1e7d974