diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java index 1f119218c8..3854310919 100644 --- a/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -123,6 +123,11 @@ public class RefNames { return shard(id.get(), r).append(META_SUFFIX).toString(); } + public static String patchSetRef(PatchSet.Id id) { + StringBuilder r = newStringBuilder().append(REFS_CHANGES); + return shard(id.changeId().get(), r).append('/').append(id.get()).toString(); + } + public static String robotCommentsRef(Change.Id id) { StringBuilder r = newStringBuilder().append(REFS_CHANGES); return shard(id.get(), r).append(ROBOT_COMMENTS_SUFFIX).toString(); diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java index 66e66ca859..b0f8e54336 100644 --- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java @@ -35,12 +35,10 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.ReceiveCommitsExecutor; -import com.google.gerrit.server.git.DefaultAdvertiseRefsHook; import com.google.gerrit.server.git.MultiProgressMonitor; import com.google.gerrit.server.git.ProjectRunnable; import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.permissions.PermissionBackend; -import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.project.ContributorAgreementsChecker; @@ -58,7 +56,6 @@ import com.google.inject.assistedinject.FactoryModuleBuilder; import com.google.inject.name.Named; import java.io.IOException; import java.io.OutputStream; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.concurrent.ExecutionException; @@ -66,8 +63,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.transport.AdvertiseRefsHook; -import org.eclipse.jgit.transport.AdvertiseRefsHookChain; import org.eclipse.jgit.transport.PreReceiveHook; import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.transport.ReceiveCommand.Result; @@ -297,15 +292,10 @@ public class AsyncReceiveCommits implements PreReceiveHook { receiveConfig.checkReferencedObjectsAreReachable); } - List advHooks = new ArrayList<>(4); allRefsWatcher = new AllRefsWatcher(); - advHooks.add(allRefsWatcher); - advHooks.add( - new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build())); - advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName)); - advHooks.add(new HackPushNegotiateHook()); - receivePack.setAdvertiseRefsHook(AdvertiseRefsHookChain.newChain(advHooks)); - + receivePack.setAdvertiseRefsHook( + ReceiveCommitsAdvertiseRefsHookChain.create( + allRefsWatcher, perm, queryProvider, projectName)); resultChangeIds = new ResultChangeIds(); receiveCommits = factory.create( diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java index f7e0078610..2ea417ec59 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java @@ -14,9 +14,8 @@ package com.google.gerrit.server.git.receive; -import com.google.auto.value.AutoValue; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.Maps; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.gerrit.exceptions.StorageException; @@ -29,28 +28,44 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.util.MagicBranch; import com.google.inject.Provider; +import java.io.IOException; import java.util.Collections; import java.util.Map; import java.util.Set; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.AdvertiseRefsHook; import org.eclipse.jgit.transport.BaseReceivePack; import org.eclipse.jgit.transport.ServiceMayNotContinueException; import org.eclipse.jgit.transport.UploadPack; -/** Exposes only the non refs/changes/ reference names. */ +/** + * Exposes only the non refs/changes/ reference names and provide additional haves. + * + *

Negotiation on Git push is suboptimal in that it tends to send more objects to the server than + * it should. This results in increased latencies for {@code git push}. + * + *

Ref advertisement for Git pushes still works in a "the server speaks first fashion" as Git + * Protocol V2 only addressed fetches Therefore the server needs to send all available references. + * For large repositories, this can be in the tens of megabytes to send to the client. We therefore + * remove all refs in refs/changes/* to scale down that footprint. Trivially, this would increase + * the unnecessary objects that the client has to send to the server because the common ancestor it + * finds in negotiation might be further back in history. + * + *

To work around this, we advertise the last 32 changes in that repository as additional {@code + * .haves}. This is a heuristical approach that aims at scaling down the number of unnecessary + * objects that client sends to the server. Unnecessary here refers to objects that the server + * already has. + * + *

For some code paths in {@link com.google.gerrit.server.git.DefaultAdvertiseRefsHook}, we + * already removed refs/changes, so the logic to skip these in this class become a no-op. + * + *

TODO(hiesel): Instrument this heuristic and proof its value. + */ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - @VisibleForTesting - @AutoValue - public abstract static class Result { - public abstract Map allRefs(); - - public abstract Set additionalHaves(); - } - private final Provider queryProvider; private final Project.NameKey projectName; @@ -68,28 +83,16 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook { @Override public void advertiseRefs(BaseReceivePack rp) throws ServiceMayNotContinueException { - Result r = advertiseRefs(HookUtil.ensureAllRefsAdvertised(rp)); - rp.setAdvertisedRefs(r.allRefs(), r.additionalHaves()); + Map advertisedRefs = HookUtil.ensureAllRefsAdvertised(rp); + advertisedRefs.keySet().stream() + .filter(ReceiveCommitsAdvertiseRefsHook::skip) + .collect(toImmutableList()) + .forEach(r -> advertisedRefs.remove(r)); + rp.setAdvertisedRefs(advertisedRefs, advertiseOpenChanges(rp.getRepository())); } - @VisibleForTesting - public Result advertiseRefs(Map oldRefs) { - Map r = Maps.newHashMapWithExpectedSize(oldRefs.size()); - Set allPatchSets = Sets.newHashSetWithExpectedSize(oldRefs.size()); - for (Map.Entry e : oldRefs.entrySet()) { - String name = e.getKey(); - if (!skip(name)) { - r.put(name, e.getValue()); - } - if (name.startsWith(RefNames.REFS_CHANGES)) { - allPatchSets.add(e.getValue().getObjectId()); - } - } - return new AutoValue_ReceiveCommitsAdvertiseRefsHook_Result( - r, advertiseOpenChanges(allPatchSets)); - } - - private Set advertiseOpenChanges(Set allPatchSets) { + private Set advertiseOpenChanges(Repository repo) + throws ServiceMayNotContinueException { // Advertise some recent open changes, in case a commit is based on one. int limit = 32; try { @@ -111,11 +114,17 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook { // Ensure we actually observed a patch set ref pointing to this // object, in case the database is out of sync with the repo and the // object doesn't actually exist. - if (allPatchSets.contains(ps.commitId())) { - r.add(ps.commitId()); + try { + Ref psRef = repo.getRefDatabase().exactRef(RefNames.patchSetRef(ps.id())); + if (psRef != null) { + r.add(ps.commitId()); + } + } catch (IOException e) { + throw new ServiceMayNotContinueException(e); } } } + return r; } catch (StorageException err) { logger.atSevere().withCause(err).log("Cannot list open changes of %s", projectName); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java new file mode 100644 index 0000000000..d10fc9a39c --- /dev/null +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java @@ -0,0 +1,76 @@ +// Copyright (C) 2019 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.google.gerrit.server.git.receive; + +import com.google.common.annotations.VisibleForTesting; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.git.DefaultAdvertiseRefsHook; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions; +import com.google.gerrit.server.query.change.InternalChangeQuery; +import com.google.inject.Provider; +import java.util.ArrayList; +import java.util.List; +import org.eclipse.jgit.transport.AdvertiseRefsHook; +import org.eclipse.jgit.transport.AdvertiseRefsHookChain; + +/** + * Helper to ensure that the chain for advertising refs is the same in tests and production code. + */ +public class ReceiveCommitsAdvertiseRefsHookChain { + + /** + * Returns a single {@link AdvertiseRefsHook} that encompasses a chain of {@link + * AdvertiseRefsHook} to be used for advertising when processing a Git push. + */ + public static AdvertiseRefsHook create( + AllRefsWatcher allRefsWatcher, + PermissionBackend.ForProject perm, + Provider queryProvider, + Project.NameKey projectName) { + return create(allRefsWatcher, perm, queryProvider, projectName, false); + } + + /** + * Returns a single {@link AdvertiseRefsHook} that encompasses a chain of {@link + * AdvertiseRefsHook} to be used for advertising when processing a Git push. Omits {@link + * HackPushNegotiateHook} as that does not advertise refs on it's own but adds {@code .have} based + * on history which is not relevant for the tests we have. + */ + @VisibleForTesting + public static AdvertiseRefsHook createForTest( + PermissionBackend.ForProject perm, + Provider queryProvider, + Project.NameKey projectName) { + return create(new AllRefsWatcher(), perm, queryProvider, projectName, true); + } + + private static AdvertiseRefsHook create( + AllRefsWatcher allRefsWatcher, + PermissionBackend.ForProject perm, + Provider queryProvider, + Project.NameKey projectName, + boolean skipHackPushNegotiateHook) { + List advHooks = new ArrayList<>(); + advHooks.add(allRefsWatcher); + advHooks.add( + new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build())); + advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName)); + if (!skipHackPushNegotiateHook) { + advHooks.add(new HackPushNegotiateHook()); + } + return AdvertiseRefsHookChain.newChain(advHooks); + } +} diff --git a/java/com/google/gerrit/server/git/receive/testing/BUILD b/java/com/google/gerrit/server/git/receive/testing/BUILD new file mode 100644 index 0000000000..82cd14bea1 --- /dev/null +++ b/java/com/google/gerrit/server/git/receive/testing/BUILD @@ -0,0 +1,12 @@ +java_library( + name = "testing", + testonly = 1, + srcs = glob(["**/*.java"]), + visibility = ["//visibility:public"], + deps = [ + "//lib:guava", + "//lib/auto:auto-value", + "//lib/auto:auto-value-annotations", + "//lib/jgit/org.eclipse.jgit:jgit", + ], +) diff --git a/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java b/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java new file mode 100644 index 0000000000..c54ab25f94 --- /dev/null +++ b/java/com/google/gerrit/server/git/receive/testing/TestRefAdvertiser.java @@ -0,0 +1,87 @@ +// Copyright (C) 2019 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.google.gerrit.server.git.receive.testing; + +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Splitter; +import java.io.IOException; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.StreamSupport; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.RefAdvertiser; + +/** Helper to collect advertised refs and additonal haves and verify them in tests. */ +public class TestRefAdvertiser extends RefAdvertiser { + + @VisibleForTesting + @AutoValue + public abstract static class Result { + public abstract Map allRefs(); + + public abstract Set additionalHaves(); + + public static Result create(Map allRefs, Set additionalHaves) { + return new AutoValue_TestRefAdvertiser_Result(allRefs, additionalHaves); + } + } + + private final Map advertisedRefs; + private final Set additionalHaves; + private final Repository repo; + + public TestRefAdvertiser(Repository repo) { + advertisedRefs = new HashMap<>(); + additionalHaves = new HashSet<>(); + this.repo = repo; + } + + @Override + protected void writeOne(CharSequence line) throws IOException { + List lineParts = + StreamSupport.stream(Splitter.on(' ').split(line).spliterator(), false) + .map(String::trim) + .collect(toImmutableList()); + if (".have".equals(lineParts.get(1))) { + additionalHaves.add(ObjectId.fromString(lineParts.get(0))); + } else { + ObjectId id = ObjectId.fromString(lineParts.get(0)); + Ref ref = + repo.getRefDatabase().getRefs().stream() + .filter(r -> r.getObjectId().equals(id)) + .findAny() + .orElseThrow( + () -> + new RuntimeException( + line.toString() + " does not conform to expected pattern")); + advertisedRefs.put(lineParts.get(1), ref); + } + } + + @Override + protected void end() {} + + public Result result() { + return Result.create(advertisedRefs, additionalHaves); + } +} diff --git a/javatests/com/google/gerrit/acceptance/git/BUILD b/javatests/com/google/gerrit/acceptance/git/BUILD index 96710e21b8..0de307ac49 100644 --- a/javatests/com/google/gerrit/acceptance/git/BUILD +++ b/javatests/com/google/gerrit/acceptance/git/BUILD @@ -8,6 +8,7 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests") ":push_for_review", ":submodule_util", "//java/com/google/gerrit/git", + "//java/com/google/gerrit/server/git/receive/testing", "//lib/commons:lang", ], ) for f in glob(["*IT.java"])] diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 811ef352e7..a3017b6f89 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -50,7 +50,8 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.receive.ReceiveCommitsAdvertiseRefsHook; +import com.google.gerrit.server.git.receive.ReceiveCommitsAdvertiseRefsHookChain; +import com.google.gerrit.server.git.receive.testing.TestRefAdvertiser; import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.notedb.Sequences; import com.google.gerrit.server.permissions.PermissionBackend; @@ -75,6 +76,8 @@ import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.AdvertiseRefsHook; +import org.eclipse.jgit.transport.ReceivePack; import org.junit.Before; import org.junit.Test; @@ -544,11 +547,10 @@ public class RefAdvertisementIT extends AbstractDaemonTest { @Test public void receivePackListsOpenChangesAsAdditionalHaves() throws Exception { - ReceiveCommitsAdvertiseRefsHook.Result r = getReceivePackRefs(); + TestRefAdvertiser.Result r = getReceivePackRefs(admin); assertThat(r.allRefs().keySet()) .containsExactly( // meta refs are excluded - "HEAD", "refs/heads/branch", "refs/heads/master", "refs/meta/config", @@ -568,7 +570,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest { .update(); requestScopeOperations.setApiUser(user.id()); - assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 1)); + assertThat(getReceivePackRefs(user).additionalHaves()).containsExactly(obj(cd3, 1)); } @Test @@ -577,7 +579,8 @@ public class RefAdvertisementIT extends AbstractDaemonTest { PushOneCommit.Result r = amendChange(cd3.change().getKey().get()); r.assertOkStatus(); cd3 = r.getChange(); - assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 2), obj(cd4, 1)); + assertThat(getReceivePackRefs(admin).additionalHaves()) + .containsExactly(obj(cd3, 2), obj(cd4, 1)); } @Test @@ -615,7 +618,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest { indexer.index(c.getProject(), c.getId()); } - assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd4, 1)); + assertThat(getReceivePackRefs(admin).additionalHaves()).containsExactly(obj(cd4, 1)); } @Test @@ -961,11 +964,16 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } } - private ReceiveCommitsAdvertiseRefsHook.Result getReceivePackRefs() throws Exception { - ReceiveCommitsAdvertiseRefsHook hook = - new ReceiveCommitsAdvertiseRefsHook(queryProvider, project); + private TestRefAdvertiser.Result getReceivePackRefs(TestAccount u) throws Exception { try (Repository repo = repoManager.openRepository(project)) { - return hook.advertiseRefs(getAllRefs(repo)); + AdvertiseRefsHook adv = + ReceiveCommitsAdvertiseRefsHookChain.createForTest( + newFilter(project, u), queryProvider, project); + ReceivePack rp = new ReceivePack(repo); + rp.setAdvertiseRefsHook(adv); + TestRefAdvertiser advertiser = new TestRefAdvertiser(repo); + rp.sendAdvertisedRefs(advertiser); + return advertiser.result(); } }