ApprovalContext: Drop checkState that verified a wrong assumption
This code made the wrong assumption that a consecutive patch set always
has a patch set number that is by 1 greater than the number of the
previous patch set.
For old changes it can be that there are gaps in the patch set numbers:
1. Due to non-atomic patch set creations with ReviewDb:
If a failure happened after creating the patch set ref and before
the patch set was created in the database, any new attempt to create
the patch was using the next number that was free according to the
existing patch set refs.
2. Due to deleted draft patch sets.
If there was a gap in the patch set numbers and approvals were supposed
to be copied over this gap, loading the change failed with an ISE [1].
A similar exception occurred when such a change was found as part of a
change query [2].
To fix this drop the checkState that makes the wrong assumption.
[1]
com.google.gerrit.exceptions.StorageException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:452)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:302)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:298)
at com.google.gerrit.server.restapi.change.GetChange.apply(GetChange.java:95)
at com.google.gerrit.server.restapi.change.GetDetail.apply(GetDetail.java:65)
at com.google.gerrit.server.restapi.change.GetDetail.apply(GetDetail.java:29)
at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestReadViewWithRetry$6(RestApiServlet.java:867)
at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
at com.github.rholder.retry.Retryer.call(Retryer.java:160)
at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:561)
at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:504)
at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:958)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestReadViewWithRetry(RestApiServlet.java:862)
at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:530)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
...
[2]
ChangeJson.toChangeInfos:516 Omitting corrupt change 8267 from results
com.google.gerrit.exceptions.StorageException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:452)
at com.google.gerrit.server.change.ChangeJson.toChangeInfos(ChangeJson.java:510)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:319)
at com.google.gerrit.server.restapi.change.QueryChanges.query(QueryChanges.java:190)
at com.google.gerrit.server.restapi.change.QueryChanges.apply(QueryChanges.java:149)
at com.google.gerrit.server.restapi.change.QueryChanges.apply(QueryChanges.java:48)
at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestReadViewWithRetry$6(RestApiServlet.java:867)
at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
at com.github.rholder.retry.Retryer.call(Retryer.java:160)
at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:561)
at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:504)
at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:958)
at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestReadViewWithRetry(RestApiServlet.java:862)
at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:530)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
...
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2052)
at com.google.common.cache.LocalCache.get(LocalCache.java:3972)
at com.google.common.cache.LocalCache$LocalManualCache.get(LocalCache.java:4869)
at com.google.devtools.gerritcodereview.common.cache.BigtableBackedCache.get(BigtableBackedCache.java:91)
at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache.get(SiteCacheFactory.java:374)
at com.google.gerrit.server.approval.ApprovalCacheImpl.get(ApprovalCacheImpl.java:74)
at com.google.gerrit.server.approval.ApprovalsUtil.byPatchSet(ApprovalsUtil.java:348)
at com.google.gerrit.server.git.MergeUtil.safeGetApprovals(MergeUtil.java:653)
at com.google.gerrit.server.git.MergeUtil.createDetailedCommitMessage(MergeUtil.java:558)
at com.google.gerrit.server.git.MergeUtil.createCommitMessageOnSubmit(MergeUtil.java:640)
at com.google.gerrit.server.change.RevisionJson.toRevisionInfo(RevisionJson.java:317)
at com.google.gerrit.server.change.RevisionJson.getRevisions(RevisionJson.java:238)
at com.google.gerrit.server.change.ChangeJson.toChangeInfoImpl(ChangeJson.java:723)
at com.google.gerrit.server.change.ChangeJson.toChangeInfo(ChangeJson.java:571)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:444)
... 220 more
Caused by: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
at com.google.common.base.Preconditions.checkState(Preconditions.java:832)
at com.google.gerrit.server.query.approval.ApprovalContext.create(ApprovalContext.java:47)
at com.google.gerrit.server.approval.ApprovalInference.canCopyBasedOnCopyCondition(ApprovalInference.java:319)
at com.google.gerrit.server.approval.ApprovalInference.getForPatchSetWithoutNormalization(ApprovalInference.java:424)
at com.google.gerrit.server.approval.ApprovalInference.forPatchSet(ApprovalInference.java:116)
at com.google.gerrit.server.approval.ApprovalCacheImpl$Loader.load(ApprovalCacheImpl.java:105)
at com.google.gerrit.server.approval.ApprovalCacheImpl$Loader.load(ApprovalCacheImpl.java:88)
at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache.lambda$get$0(SiteCacheFactory.java:372)
at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$RecordingCallable.lambda$new$0(SiteCacheFactory.java:571)
at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$BulkRecordingCallable.call(SiteCacheFactory.java:609)
at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$RecordingCallable.call(SiteCacheFactory.java:577)
at com.google.devtools.gerritcodereview.common.cache.BigtableBackedCache$Loader.call(BigtableBackedCache.java:216)
at com.google.common.cache.LocalCache$LocalManualCache$1.load(LocalCache.java:4874)
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3539)
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2279)
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2156)
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2046)
... 234 more
Issue: Google b/194181275
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I995e213054a252f46c68c8fe53d0a85691f06adb
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
index 4c2c7e8..3bf072a 100644
--- a/java/com/google/gerrit/server/query/approval/ApprovalContext.java
+++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -44,11 +44,12 @@
"approval and target must be the same change. got: %s, %s",
psa.patchSetId(),
id);
- checkState(
- psa.patchSetId().get() + 1 == id.get(),
- "approvals can only be copied to the next consecutive patch set. got: %s, %s",
- psa.patchSetId(),
- id);
+ // TODO(ekempin): Use checkState to verify that psa.patchSetId().get() + 1 == id.get() so that
+ // it's ensured that approvals are only copied to the next consecutive patch set. To add back
+ // this verification https://gerrit-review.googlesource.com/c/gerrit/+/312633 can be reverted.
+ // As explained in the commit message of this change doing this check is only possible if there
+ // are no changes with gaps in patch set numbers. Since it's planned to fix-up old changes with
+ // gaps in patch set numbers, this todo is a reminder to add back the check once this is done.
return new AutoValue_ApprovalContext(psa, id, changeNotes, changeKind);
}
}
