opendev/gerrit
Code Issues Proposed changes

SubscriptionGraph: Inject SubscriptionGraph.Factory

Browse Source 
Use Guice to inject SubscriptionGraph.Factory instead of calling
constructor directly, which allows an alternative implementation of
SubmoduleGraph.Factory being injected easily.

Change-Id: I689998da72e3787bf36a2c854c40f206fabacf9e
This commit is contained in:
Yunjie Li
2020-05-19 19:34:11 -07:00
parent 0a633ea1cf
commit 3313e5f4b9
5 changed files with 50 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/pgm/Daemon.java
View File

@@ -107,6 +107,7 @@ import com.google.gerrit.server.ssh.NoSshKeyCache;
import com.google.gerrit.server.ssh.NoSshModule;
import com.google.gerrit.server.ssh.SshAddressesModule;
import com.google.gerrit.server.submit.LocalMergeSuperSetComputation;
import com.google.gerrit.server.submit.SubscriptionGraph;
import com.google.gerrit.sshd.SshHostKeyModule;
import com.google.gerrit.sshd.SshKeyCacheImpl;
import com.google.gerrit.sshd.SshModule;
@@ -411,6 +412,7 @@ public class Daemon extends SiteProgram {
// work queue can get stuck waiting on index futures that will never return.
modules.add(createIndexModule());
modules.add(new SubscriptionGraph.Module());
modules.add(new WorkQueue.Module());
modules.add(new StreamEventsApiListener.Module());
modules.add(new EventBroker.Module());

22
java/com/google/gerrit/server/submit/SubmoduleOp.java
View File

@@ -30,7 +30,6 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.VerboseSuperprojectUpdate;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateListener;
@@ -89,27 +88,24 @@ public class SubmoduleOp {
@Singleton
public static class Factory {
private final GitModules.Factory gitmodulesFactory;
private final SubscriptionGraph.Factory subscriptionGraphFactory;
private final Provider<PersonIdent> serverIdent;
private final Config cfg;
private final ProjectCache projectCache;
@Inject
Factory(
GitModules.Factory gitmodulesFactory,
SubscriptionGraph.Factory subscriptionGraphFactory,
@GerritPersonIdent Provider<PersonIdent> serverIdent,
@GerritServerConfig Config cfg,
ProjectCache projectCache) {
this.gitmodulesFactory = gitmodulesFactory;
@GerritServerConfig Config cfg) {
this.subscriptionGraphFactory = subscriptionGraphFactory;
this.serverIdent = serverIdent;
this.cfg = cfg;
this.projectCache = projectCache;
}
public SubmoduleOp create(Set<BranchNameKey> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleConflictException {
return new SubmoduleOp(
gitmodulesFactory, serverIdent.get(), cfg, projectCache, updatedBranches, orm);
subscriptionGraphFactory, serverIdent.get(), cfg, updatedBranches, orm);
}
}
@@ -118,16 +114,14 @@ public class SubmoduleOp {
private final long maxCombinedCommitMessageSize;
private final long maxCommitMessages;
private final MergeOpRepoManager orm;
private final SubscriptionGraph.Factory subscriptionGraphFactory;
private final SubscriptionGraph subscriptionGraph;
private final BranchTips branchTips = new BranchTips();
private SubmoduleOp(
GitModules.Factory gitmodulesFactory,
SubscriptionGraph.Factory subscriptionGraphFactory,
PersonIdent myIdent,
Config cfg,
ProjectCache projectCache,
Set<BranchNameKey> updatedBranches,
MergeOpRepoManager orm)
throws SubmoduleConflictException {
@@ -138,10 +132,8 @@ public class SubmoduleOp {
cfg.getLong("submodule", "maxCombinedCommitMessageSize", 256 << 10);
this.maxCommitMessages = cfg.getLong("submodule", "maxCommitMessages", 1000);
this.orm = orm;
this.subscriptionGraphFactory =
new SubscriptionGraph.DefaultFactory(gitmodulesFactory, projectCache, orm);
if (cfg.getBoolean("submodule", "enableSuperProjectSubscriptions", true)) {
this.subscriptionGraph = subscriptionGraphFactory.compute(updatedBranches);
this.subscriptionGraph = subscriptionGraphFactory.compute(updatedBranches, orm);
} else {
logger.atFine().log("Updating superprojects disabled");
this.subscriptionGraph =

47
java/com/google/gerrit/server/submit/SubscriptionGraph.java
View File

@@ -31,6 +31,8 @@ import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayDeque;
import java.util.ArrayList;
@@ -150,24 +152,30 @@ public class SubscriptionGraph {
}
public interface Factory {
SubscriptionGraph compute(Set<BranchNameKey> updatedBranches) throws SubmoduleConflictException;
SubscriptionGraph compute(Set<BranchNameKey> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleConflictException;
}
public static class Module extends AbstractModule {
@Override
protected void configure() {
bind(Factory.class).to(DefaultFactory.class);
}
}
static class DefaultFactory implements Factory {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ProjectCache projectCache;
private final GitModules.Factory gitmodulesFactory;
private final MergeOpRepoManager orm;
DefaultFactory(
GitModules.Factory gitmodulesFactory, ProjectCache projectCache, MergeOpRepoManager orm) {
@Inject
DefaultFactory(GitModules.Factory gitmodulesFactory, ProjectCache projectCache) {
this.gitmodulesFactory = gitmodulesFactory;
this.projectCache = projectCache;
this.orm = orm;
}
@Override
public SubscriptionGraph compute(Set<BranchNameKey> updatedBranches)
public SubscriptionGraph compute(Set<BranchNameKey> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleConflictException {
Map<BranchNameKey, GitModules> branchGitModules = new HashMap<>();
// All affected branches, including those in superprojects and submodules.
@@ -191,7 +199,8 @@ public class SubscriptionGraph {
targets,
branchesByProject,
subscribedBranches,
branchGitModules);
branchGitModules,
orm);
return new SubscriptionGraph(
updatedBranches, targets, branchesByProject, subscribedBranches, sortedBranches);
@@ -217,7 +226,8 @@ public class SubscriptionGraph {
SetMultimap<BranchNameKey, SubmoduleSubscription> targets,
SetMultimap<Project.NameKey, BranchNameKey> branchesByProject,
Set<BranchNameKey> subscribedBranches,
Map<BranchNameKey, GitModules> branchGitModules)
Map<BranchNameKey, GitModules> branchGitModules,
MergeOpRepoManager orm)
throws SubmoduleConflictException {
logger.atFine().log("Calculating superprojects - submodules map");
LinkedHashSet<BranchNameKey> allVisited = new LinkedHashSet<>();
@@ -234,7 +244,8 @@ public class SubscriptionGraph {
targets,
branchesByProject,
subscribedBranches,
branchGitModules);
branchGitModules,
orm);
}
// Since the searchForSuperprojects will add all branches (related or
@@ -254,7 +265,8 @@ public class SubscriptionGraph {
SetMultimap<BranchNameKey, SubmoduleSubscription> targets,
SetMultimap<Project.NameKey, BranchNameKey> branchesByProject,
Set<BranchNameKey> subscribedBranches,
Map<BranchNameKey, GitModules> branchGitModules)
Map<BranchNameKey, GitModules> branchGitModules,
MergeOpRepoManager orm)
throws SubmoduleConflictException {
logger.atFine().log("Now processing %s", current);
@@ -271,7 +283,7 @@ public class SubscriptionGraph {
currentVisited.add(current);
try {
Collection<SubmoduleSubscription> subscriptions =
superProjectSubscriptionsForSubmoduleBranch(current, branchGitModules);
superProjectSubscriptionsForSubmoduleBranch(current, branchGitModules, orm);
for (SubmoduleSubscription sub : subscriptions) {
BranchNameKey superBranch = sub.getSuperProject();
searchForSuperprojects(
@@ -282,7 +294,8 @@ public class SubscriptionGraph {
targets,
branchesByProject,
subscribedBranches,
branchGitModules);
branchGitModules,
orm);
targets.put(superBranch, sub);
branchesByProject.put(superBranch.project(), superBranch);
affectedBranches.add(superBranch);
@@ -296,8 +309,8 @@ public class SubscriptionGraph {
allVisited.add(current);
}
private Collection<BranchNameKey> getDestinationBranches(BranchNameKey src, SubscribeSection s)
throws IOException {
private Collection<BranchNameKey> getDestinationBranches(
BranchNameKey src, SubscribeSection s, MergeOpRepoManager orm) throws IOException {
OpenRepo or;
try {
or = orm.getRepo(s.project());
@@ -313,7 +326,9 @@ public class SubscriptionGraph {
}
private Collection<SubmoduleSubscription> superProjectSubscriptionsForSubmoduleBranch(
BranchNameKey srcBranch, Map<BranchNameKey, GitModules> branchGitModules)
BranchNameKey srcBranch,
Map<BranchNameKey, GitModules> branchGitModules,
MergeOpRepoManager orm)
throws IOException {
logger.atFine().log("Calculating possible superprojects for %s", srcBranch);
Collection<SubmoduleSubscription> ret = new ArrayList<>();
@@ -324,7 +339,7 @@ public class SubscriptionGraph {
.orElseThrow(illegalState(srcProject))
.getSubscribeSections(srcBranch)) {
logger.atFine().log("Checking subscribe section %s", s);
Collection<BranchNameKey> branches = getDestinationBranches(srcBranch, s);
Collection<BranchNameKey> branches = getDestinationBranches(srcBranch, s, orm);
for (BranchNameKey targetBranch : branches) {
Project.NameKey targetProject = targetBranch.project();
try {

2
java/com/google/gerrit/testing/InMemoryModule.java
View File

@@ -87,6 +87,7 @@ import com.google.gerrit.server.securestore.DefaultSecureStore;
import com.google.gerrit.server.securestore.SecureStore;
import com.google.gerrit.server.ssh.NoSshKeyCache;
import com.google.gerrit.server.submit.LocalMergeSuperSetComputation;
import com.google.gerrit.server.submit.SubscriptionGraph;
import com.google.gerrit.server.util.ReplicaUtil;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
@@ -175,6 +176,7 @@ public class InMemoryModule extends FactoryModule {
install(new SearchingChangeCacheImpl.Module());
factory(GarbageCollection.Factory.class);
install(new AuditModule());
install(new SubscriptionGraph.Module());
bindScope(RequestScoped.class, PerThreadRequestScope.REQUEST);

17
javatests/com/google/gerrit/server/submit/SubscriptionGraphTest.java
View File

@@ -81,9 +81,9 @@ public class SubscriptionGraphTest {
@Test
public void oneSuperprojectOneSubmodule() throws Exception {
SubscriptionGraph.Factory factory =
new DefaultFactory(mockGitModulesFactory, mockProjectCache, mergeOpRepoManager);
SubscriptionGraph subscriptionGraph = factory.compute(ImmutableSet.of(SUB_BRANCH));
SubscriptionGraph.Factory factory = new DefaultFactory(mockGitModulesFactory, mockProjectCache);
SubscriptionGraph subscriptionGraph =
factory.compute(ImmutableSet.of(SUB_BRANCH), mergeOpRepoManager);
assertThat(subscriptionGraph.getAffectedSuperProjects()).containsExactly(SUPER_PROJECT);
assertThat(subscriptionGraph.getAffectedSuperBranches(SUPER_PROJECT))
@@ -98,12 +98,12 @@ public class SubscriptionGraphTest {
@Test
public void circularSubscription() throws Exception {
SubscriptionGraph.Factory factory =
new DefaultFactory(mockGitModulesFactory, mockProjectCache, mergeOpRepoManager);
SubscriptionGraph.Factory factory = new DefaultFactory(mockGitModulesFactory, mockProjectCache);
setSubscription(SUPER_BRANCH, ImmutableList.of(SUB_BRANCH));
SubmoduleConflictException e =
assertThrows(
SubmoduleConflictException.class, () -> factory.compute(ImmutableSet.of(SUB_BRANCH)));
SubmoduleConflictException.class,
() -> factory.compute(ImmutableSet.of(SUB_BRANCH), mergeOpRepoManager));
String expectedErrorMessage =
"Subproject,refs/heads/one->Superproject,refs/heads/one->Subproject,refs/heads/one";
@@ -154,10 +154,9 @@ public class SubscriptionGraphTest {
setSubscription(submoduleBranch2, ImmutableList.of(superBranch1));
setSubscription(submoduleBranch3, ImmutableList.of(superBranch1, superBranch2));
SubscriptionGraph.Factory factory =
new DefaultFactory(mockGitModulesFactory, mockProjectCache, mergeOpRepoManager);
SubscriptionGraph.Factory factory = new DefaultFactory(mockGitModulesFactory, mockProjectCache);
SubscriptionGraph subscriptionGraph =
factory.compute(ImmutableSet.of(submoduleBranch1, submoduleBranch2));
factory.compute(ImmutableSet.of(submoduleBranch1, submoduleBranch2), mergeOpRepoManager);
assertThat(subscriptionGraph.getAffectedSuperProjects())
.containsExactly(superProject1, superProject2);