Use gerrit.instanceId instead of replication.instanceLabel Pull-replication uses replication.instanceLabel to identify source node. Gerrit 3.4 introduce gerrit.instanceId property. To avoid having multiple properties which contains similar information use gerrit.instanceId instead of replication.instanceLabel. For backward compatibility replication.instanceLabel is still used if present if not gerrit.instanceId will be used. Bug: Issue 15636 Change-Id: I324f25bc38007a2a48c1068980a31c637f2cd9d1
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java index f2f57cd..f1d486d 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClient.java
@@ -25,6 +25,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.restapi.Url; +import com.google.gerrit.server.config.GerritInstanceId; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.inject.Inject; @@ -66,7 +67,7 @@ private final CredentialsFactory credentials; private final SourceHttpClient.Factory httpClientFactory; private final Source source; - private final String instanceLabel; + private final String instanceId; private final String pluginName; private final SyncRefsFilter syncRefsFilter; @@ -77,18 +78,22 @@ ReplicationConfig replicationConfig, SyncRefsFilter syncRefsFilter, @PluginName String pluginName, + @Nullable @GerritInstanceId String instanceId, @Assisted Source source) { this.credentials = credentials; this.httpClientFactory = httpClientFactory; this.source = source; this.pluginName = pluginName; this.syncRefsFilter = syncRefsFilter; - this.instanceLabel = - Strings.nullToEmpty( + this.instanceId = + Optional.ofNullable( replicationConfig.getConfig().getString("replication", null, "instanceLabel")) + .orElse(instanceId) .trim(); + requireNonNull( - Strings.emptyToNull(instanceLabel), "replication.instanceLabel cannot be null or empty"); + Strings.emptyToNull(this.instanceId), + "gerrit.instanceId or replication.instanceLabel must be set"); } /* (non-Javadoc) @@ -107,7 +112,7 @@ new StringEntity( String.format( "{\"label\":\"%s\", \"ref_name\": \"%s\", \"async\":%s}", - instanceLabel, refName, callAsync), + instanceId, refName, callAsync), StandardCharsets.UTF_8)); post.addHeader(new BasicHeader("Content-Type", "application/json")); return httpClientFactory.create(source).execute(post, this, getContext(targetUri)); @@ -172,7 +177,7 @@ } else { requireNull(revisionData, "DELETE ref-updates cannot be associated with a RevisionData"); } - RevisionInput input = new RevisionInput(instanceLabel, refName, revisionData); + RevisionInput input = new RevisionInput(instanceId, refName, revisionData); String url = String.format(
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index b2470a4..53541ba 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -31,8 +31,13 @@ threads = 3 authGroup = Public Mirror Group authGroup = Second Public Mirror Group - [replication] - instanceLabel = host-one +``` + +And make sure that instanceId is setup in `$site_path/etc/gerrit.config`: + +``` +[gerrit] + instanceId = host-one ``` Then reload the replication plugin to pick up the new configuration: @@ -121,6 +126,10 @@ provided in the remote configuration section which name is equal to instanceLabel. + Deprecated: This property is kept for backward compatibility and + will be removed in the future release. Use [gerrit.instanceId](https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#gerrit.instanceId) + instead. + replication.maxConnectionsPerRoute : Maximum number of HTTP connections per one HTTP route. @@ -441,7 +450,6 @@ autoReload = true replicateOnStartup = false [replication] - instanceLabel = host-one lockErrorMaxRetries = 5 maxRetries = 5 ``` @@ -477,7 +485,6 @@ autoReload = true replicateOnStartup = false [replication] - instanceLabel = host-one lockErrorMaxRetries = 5 maxRetries = 5
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationFanoutConfigIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationFanoutConfigIT.java index cb5af42..ee5876f 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationFanoutConfigIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationFanoutConfigIT.java
@@ -23,6 +23,7 @@ import com.google.gerrit.acceptance.SkipProjectClone; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.api.projects.BranchInput; @@ -95,6 +96,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewChangeRef() throws Exception { testRepo = cloneProject(createTestProject(project + TEST_REPLICATION_SUFFIX)); @@ -122,6 +124,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewChangeRefAfterConfigReloaded() throws Exception { testRepo = cloneProject(createTestProject(project + TEST_REPLICATION_SUFFIX)); @@ -157,6 +160,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewBranch() throws Exception { String testProjectName = project + TEST_REPLICATION_SUFFIX; createTestProject(testProjectName); @@ -190,6 +194,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldAutoReloadConfiguration() throws Exception { SourcesCollection sources = getInstance(SourcesCollection.class); AutoReloadConfigDecorator autoReloadConfigDecorator = @@ -202,6 +207,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldAutoReloadConfigurationWhenRemoteConfigAdded() throws Exception { FileBasedConfig newRemoteConfig = new FileBasedConfig( @@ -224,6 +230,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldAutoReloadConfigurationWhenRemoteConfigDeleted() throws Exception { SourcesCollection sources = getInstance(SourcesCollection.class); AutoReloadConfigDecorator autoReloadConfigDecorator = @@ -259,7 +266,6 @@ } private void setReplicationSource(String remoteName) throws IOException { - config.setString("replication", null, "instanceLabel", remoteName); config.setBoolean("gerrit", null, "autoReload", true); config.save(); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationIT.java index cf94086..13b3460 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationIT.java
@@ -27,6 +27,7 @@ import com.google.gerrit.acceptance.SkipProjectClone; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; @@ -115,6 +116,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewChangeRef() throws Exception { testRepo = cloneProject(createTestProject(project + TEST_REPLICATION_SUFFIX)); @@ -142,6 +144,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewBranch() throws Exception { String testProjectName = project + TEST_REPLICATION_SUFFIX; createTestProject(testProjectName); @@ -176,6 +179,7 @@ @Test @UseLocalDisk + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateForceUpdatedBranch() throws Exception { boolean forcedPush = true; String testProjectName = project + TEST_REPLICATION_SUFFIX; @@ -245,6 +249,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewChangeRefCGitClient() throws Exception { AutoReloadConfigDecorator autoReloadConfigDecorator = getInstance(AutoReloadConfigDecorator.class); @@ -280,6 +285,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateNewBranchCGitClient() throws Exception { AutoReloadConfigDecorator autoReloadConfigDecorator = getInstance(AutoReloadConfigDecorator.class); @@ -321,6 +327,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateProjectDeletion() throws Exception { String projectToDelete = project.get(); setReplicationSource(TEST_REPLICATION_REMOTE, "", Optional.of(projectToDelete)); @@ -349,6 +356,7 @@ } @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) public void shouldReplicateHeadUpdate() throws Exception { String testProjectName = project.get(); setReplicationSource(TEST_REPLICATION_REMOTE, "", Optional.of(testProjectName)); @@ -407,7 +415,6 @@ replicaSuffixes.stream() .map(suffix -> gitPath.resolve("${name}" + suffix + ".git").toString()) .collect(toList()); - config.setString("replication", null, "instanceLabel", remoteName); config.setStringList("remote", remoteName, "url", replicaUrls); config.setString("remote", remoteName, "apiUrl", adminRestSession.url()); config.setString("remote", remoteName, "fetch", "+refs/*:refs/*");
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java index cef1051..e6f788a 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/api/ActionITBase.java
@@ -199,7 +199,6 @@ replicaSuffixes.stream() .map(suffix -> gitPath.resolve("${name}" + suffix + ".git").toString()) .collect(toList()); - config.setString("replication", null, "instanceLabel", remoteName); config.setStringList("remote", remoteName, "url", replicaUrls); config.setString("remote", remoteName, "apiUrl", adminRestSession.url()); config.setString("remote", remoteName, "fetch", "+refs/tags/*:refs/tags/*");
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java index a1c2172..f0b8b99 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientTest.java
@@ -79,7 +79,7 @@ String api = "http://gerrit-host"; String pluginName = "pull-replication"; - String label = "Replication"; + String instanceId = "Replication"; String refName = RefNames.REFS_HEADS + "master"; String expectedPayload = @@ -159,7 +159,6 @@ when(replicationConfig.getConfig()).thenReturn(config); when(config.getStringList("replication", null, "syncRefs")).thenReturn(new String[0]); when(source.getRemoteConfigName()).thenReturn("Replication"); - when(config.getString("replication", null, "instanceLabel")).thenReturn(label); HttpResult httpResult = new HttpResult(SC_CREATED, Optional.of("result message")); when(httpClient.execute(any(HttpPost.class), any(), any())).thenReturn(httpResult); @@ -167,7 +166,13 @@ syncRefsFilter = new SyncRefsFilter(replicationConfig); objectUnderTest = new FetchRestApiClient( - credentials, httpClientFactory, replicationConfig, syncRefsFilter, pluginName, source); + credentials, + httpClientFactory, + replicationConfig, + syncRefsFilter, + pluginName, + instanceId, + source); } @Test @@ -191,7 +196,13 @@ syncRefsFilter = new SyncRefsFilter(replicationConfig); objectUnderTest = new FetchRestApiClient( - credentials, httpClientFactory, replicationConfig, syncRefsFilter, pluginName, source); + credentials, + httpClientFactory, + replicationConfig, + syncRefsFilter, + pluginName, + instanceId, + source); objectUnderTest.callFetch(Project.nameKey("test_repo"), refName, new URIish(api)); @@ -210,7 +221,13 @@ syncRefsFilter = new SyncRefsFilter(replicationConfig); objectUnderTest = new FetchRestApiClient( - credentials, httpClientFactory, replicationConfig, syncRefsFilter, pluginName, source); + credentials, + httpClientFactory, + replicationConfig, + syncRefsFilter, + pluginName, + instanceId, + source); objectUnderTest.callFetch(Project.nameKey("test_repo"), refName, new URIish(api)); @@ -232,7 +249,13 @@ syncRefsFilter = new SyncRefsFilter(replicationConfig); objectUnderTest = new FetchRestApiClient( - credentials, httpClientFactory, replicationConfig, syncRefsFilter, pluginName, source); + credentials, + httpClientFactory, + replicationConfig, + syncRefsFilter, + pluginName, + instanceId, + source); objectUnderTest.callFetch(Project.nameKey("test_repo"), refName, new URIish(api)); verify(httpClient, times(1)).execute(httpPostCaptor.capture(), any(), any()); @@ -321,7 +344,6 @@ @Test public void shouldThrowExceptionWhenInstanceLabelIsNull() { - when(config.getString("replication", null, "instanceLabel")).thenReturn(null); assertThrows( NullPointerException.class, () -> @@ -331,12 +353,12 @@ replicationConfig, syncRefsFilter, pluginName, + null, source)); } @Test public void shouldTrimInstanceLabel() { - when(config.getString("replication", null, "instanceLabel")).thenReturn(" "); assertThrows( NullPointerException.class, () -> @@ -346,12 +368,12 @@ replicationConfig, syncRefsFilter, pluginName, + " ", source)); } @Test public void shouldThrowExceptionWhenInstanceLabelIsEmpty() { - when(config.getString("replication", null, "instanceLabel")).thenReturn(""); assertThrows( NullPointerException.class, () -> @@ -361,10 +383,32 @@ replicationConfig, syncRefsFilter, pluginName, + "", source)); } @Test + public void shouldUseReplicationLabelWhenProvided() + throws ClientProtocolException, IOException, URISyntaxException { + when(config.getString("replication", null, "instanceLabel")).thenReturn(instanceId); + FetchRestApiClient objectUnderTest = + new FetchRestApiClient( + credentials, + httpClientFactory, + replicationConfig, + syncRefsFilter, + pluginName, + "", + source); + objectUnderTest.callFetch(Project.nameKey("test_repo"), refName, new URIish(api)); + + verify(httpClient, times(1)).execute(httpPostCaptor.capture(), any(), any()); + + HttpPost httpPost = httpPostCaptor.getValue(); + assertThat(readPayload(httpPost)).isEqualTo(expectedPayload); + } + + @Test public void shouldCallInitProjectEndpoint() throws IOException, URISyntaxException { objectUnderTest.initProject(Project.nameKey("test_repo"), new URIish(api));