opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15' into stable-2.16

Browse Source 
* stable-2.15:
  Elasticsearch: Base the default number of shards on ES version
  Disallow change index task duplication
  AbstractPushForReview: Add tests for pushing with skip-validation option
  Add extension point to gr-user-header

Change-Id: I6de8e61e110046d645c6da0bb71923d491a5764a
This commit is contained in:
David Pursehouse
2019-06-19 10:37:40 +09:00
parent 87b2bc6919 082139a71d
commit 5f4e92627b
9 changed files with 190 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
Documentation/config-gerrit.txt
View File

@@ -3055,7 +3055,7 @@ Sets the number of shards to use per index. Refer to the
link:https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-concepts.html#getting-started-shards-and-replicas[
Elasticsearch documentation] for details.
+
Defaults to 5.
Defaults to 5 for Elasticsearch versions 5 and 6, and to 1 starting with Elasticsearch 7.
[[elasticsearch.numberOfReplicas]]elasticsearch.numberOfReplicas::
+

6
java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
View File

@@ -187,7 +187,7 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
}
// Recreate the index.
String indexCreationFields = concatJsonString(getSettings(), getMappings());
String indexCreationFields = concatJsonString(getSettings(client.adapter()), getMappings());
response =
performRequest(
"PUT", indexName + client.adapter().includeTypeNameParam(), indexCreationFields);
@@ -202,8 +202,8 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
protected abstract String getMappings();
private String getSettings() {
return gson.toJson(ImmutableMap.of(SETTINGS, ElasticSetting.createSetting(config)));
private String getSettings(ElasticQueryAdapter adapter) {
return gson.toJson(ImmutableMap.of(SETTINGS, ElasticSetting.createSetting(config, adapter)));
}
protected abstract String getId(V v);

9
java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
View File

@@ -42,7 +42,7 @@ class ElasticConfiguration {
static final String KEY_NUMBER_OF_REPLICAS = "numberOfReplicas";
static final String DEFAULT_PORT = "9200";
static final String DEFAULT_USERNAME = "elastic";
static final int DEFAULT_NUMBER_OF_SHARDS = 5;
static final int DEFAULT_NUMBER_OF_SHARDS = 0;
static final int DEFAULT_NUMBER_OF_REPLICAS = 1;
private final Config cfg;
@@ -100,4 +100,11 @@ class ElasticConfiguration {
String getIndexName(String name, int schemaVersion) {
return String.format("%s%s_%04d", prefix, name, schemaVersion);
}
int getNumberOfShards(ElasticQueryAdapter adapter) {
if (numberOfShards == DEFAULT_NUMBER_OF_SHARDS) {
return adapter.getDefaultNumberOfShards();
}
return numberOfShards;
}
}

6
java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
View File

@@ -27,6 +27,7 @@ public class ElasticQueryAdapter {
private final boolean useV5Type;
private final boolean useV6Type;
private final boolean omitType;
private final int defaultNumberOfShards;
private final String searchFilteringName;
private final String indicesExistParams;
@@ -41,6 +42,7 @@ public class ElasticQueryAdapter {
this.useV5Type = !version.isV6OrLater();
this.useV6Type = version.isV6();
this.omitType = version.isV7OrLater();
this.defaultNumberOfShards = version.isV7OrLater() ? 1 : 5;
this.versionDiscoveryUrl = version.isV6OrLater() ? "/%s*" : "/%s*/_aliases";
this.searchFilteringName = "_source";
this.indicesExistParams =
@@ -98,6 +100,10 @@ public class ElasticQueryAdapter {
return omitType;
}
int getDefaultNumberOfShards() {
return defaultNumberOfShards;
}
String getType() {
return getType("");
}

8
java/com/google/gerrit/elasticsearch/ElasticSetting.java
View File

@@ -22,18 +22,18 @@ class ElasticSetting {
private static final ImmutableMap<String, String> CUSTOM_CHAR_MAPPING =
ImmutableMap.of("\\u002E", "\\u0020", "\\u005F", "\\u0020");
static SettingProperties createSetting(ElasticConfiguration config) {
return new ElasticSetting.Builder().addCharFilter().addAnalyzer().build(config);
static SettingProperties createSetting(ElasticConfiguration config, ElasticQueryAdapter adapter) {
return new ElasticSetting.Builder().addCharFilter().addAnalyzer().build(config, adapter);
}
static class Builder {
private final ImmutableMap.Builder<String, FieldProperties> fields =
new ImmutableMap.Builder<>();
SettingProperties build(ElasticConfiguration config) {
SettingProperties build(ElasticConfiguration config, ElasticQueryAdapter adapter) {
SettingProperties properties = new SettingProperties();
properties.analysis = fields.build();
properties.numberOfShards = config.numberOfShards;
properties.numberOfShards = config.getNumberOfShards(adapter);
properties.numberOfReplicas = config.numberOfReplicas;
return properties;
}

62
java/com/google/gerrit/server/index/change/ChangeIndexer.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.index.change;
import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
import com.google.common.base.Objects;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.Atomics;
import com.google.common.util.concurrent.Futures;
@@ -53,7 +54,9 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -97,6 +100,11 @@ public class ChangeIndexer {
private final StalenessChecker stalenessChecker;
private final boolean autoReindexIfStale;
private final Set<IndexTask> queuedIndexTasks =
Collections.newSetFromMap(new ConcurrentHashMap<>());
private final Set<ReindexIfStaleTask> queuedReindexIfStaleTasks =
Collections.newSetFromMap(new ConcurrentHashMap<>());
@AssistedInject
ChangeIndexer(
@GerritServerConfig Config cfg,
@@ -164,7 +172,11 @@ public class ChangeIndexer {
@SuppressWarnings("deprecation")
public com.google.common.util.concurrent.CheckedFuture<?, IOException> indexAsync(
Project.NameKey project, Change.Id id) {
return submit(new IndexTask(project, id));
IndexTask task = new IndexTask(project, id);
if (queuedIndexTasks.add(task)) {
return submit(task);
}
return Futures.immediateCheckedFuture(null);
}
/**
@@ -288,7 +300,11 @@ public class ChangeIndexer {
@SuppressWarnings("deprecation")
public com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStale(
Project.NameKey project, Change.Id id) {
return submit(new ReindexIfStaleTask(project, id), batchExecutor);
ReindexIfStaleTask task = new ReindexIfStaleTask(project, id);
if (queuedReindexIfStaleTasks.add(task)) {
return submit(task, batchExecutor);
}
return Futures.immediateCheckedFuture(false);
}
private void autoReindexIfStale(ChangeData cd) {
@@ -331,6 +347,8 @@ public class ChangeIndexer {
protected abstract T callImpl(Provider<ReviewDb> db) throws Exception;
protected abstract void remove();
@Override
public abstract String toString();
@@ -383,15 +401,35 @@ public class ChangeIndexer {
@Override
public Void callImpl(Provider<ReviewDb> db) throws Exception {
remove();
ChangeData cd = newChangeData(db.get(), project, id);
index(cd);
return null;
}
@Override
public int hashCode() {
return Objects.hashCode(IndexTask.class, id.get());
}
@Override
public boolean equals(Object obj) {
if (!(obj instanceof IndexTask)) {
return false;
}
IndexTask other = (IndexTask) obj;
return id.get() == other.id.get();
}
@Override
public String toString() {
return "index-change-" + id;
}
@Override
protected void remove() {
queuedIndexTasks.remove(this);
}
}
// Not AbstractIndexTask as it doesn't need ReviewDb.
@@ -427,6 +465,7 @@ public class ChangeIndexer {
@Override
public Boolean callImpl(Provider<ReviewDb> db) throws Exception {
remove();
try {
if (stalenessChecker.isStale(id)) {
indexImpl(newChangeData(db.get(), project, id));
@@ -445,10 +484,29 @@ public class ChangeIndexer {
return false;
}
@Override
public int hashCode() {
return Objects.hashCode(ReindexIfStaleTask.class, id.get());
}
@Override
public boolean equals(Object obj) {
if (!(obj instanceof ReindexIfStaleTask)) {
return false;
}
ReindexIfStaleTask other = (ReindexIfStaleTask) obj;
return id.get() == other.id.get();
}
@Override
public String toString() {
return "reindex-if-stale-change-" + id;
}
@Override
protected void remove() {
queuedReindexIfStaleTasks.remove(this);
}
}
private boolean isCausedByRepositoryNotFoundException(Throwable throwable) {

40
java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.index.change;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static com.google.gerrit.server.query.change.ChangeData.asChanges;
import com.google.common.base.Objects;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
@@ -44,8 +45,11 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import org.eclipse.jgit.lib.Config;
@@ -63,6 +67,8 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
private final ListeningExecutorService executor;
private final boolean enabled;
private final Set<Index> queuedIndexTasks = Collections.newSetFromMap(new ConcurrentHashMap<>());
@Inject
ReindexAfterRefUpdate(
@GerritServerConfig Config cfg,
@@ -113,9 +119,12 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
@Override
public void onSuccess(List<Change> changes) {
for (Change c : changes) {
// Don't retry indefinitely; if this fails changes may be stale.
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = executor.submit(new Index(event, c.getId()));
Index task = new Index(event, c.getId());
if (queuedIndexTasks.add(task)) {
// Don't retry indefinitely; if this fails changes may be stale.
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError = executor.submit(task);
}
}
}
@@ -145,6 +154,8 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
}
protected abstract V impl(RequestContext ctx) throws Exception;
protected abstract void remove();
}
private class GetChanges extends Task<List<Change>> {
@@ -169,6 +180,9 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
+ " update of project "
+ event.getProjectName();
}
@Override
protected void remove() {}
}
private class Index extends Task<Void> {
@@ -183,6 +197,7 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
protected Void impl(RequestContext ctx) throws OrmException, IOException {
// Reload change, as some time may have passed since GetChanges.
ReviewDb db = ctx.getReviewDbProvider().get();
remove();
try {
Change c =
notesFactory
@@ -195,9 +210,28 @@ public class ReindexAfterRefUpdate implements GitReferenceUpdatedListener {
return null;
}
@Override
public int hashCode() {
return Objects.hashCode(Index.class, id.get());
}
@Override
public boolean equals(Object obj) {
if (!(obj instanceof Index)) {
return false;
}
Index other = (Index) obj;
return id.get() == other.id.get();
}
@Override
public String toString() {
return "Index change " + id.get() + " of project " + event.getProjectName();
}
@Override
protected void remove() {
queuedIndexTasks.remove(this);
}
}
}

62
javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -77,6 +77,8 @@ import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.common.testing.EditInfoSubject;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.mail.Address;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
@@ -87,16 +89,22 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.receive.NoteDbPushOption;
import com.google.gerrit.server.git.receive.ReceiveConstants;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.CommitValidators.ChangeIdValidator;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.testing.Util;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
@@ -104,6 +112,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import org.eclipse.jgit.api.errors.GitAPIException;
@@ -133,6 +142,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
private LabelType patchSetLock;
@Inject private DynamicSet<CommitValidationListener> commitValidators;
@BeforeClass
public static void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -2255,6 +2266,57 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
.isEqualTo(Iterables.getLast(commits).name());
}
private static class TestValidator implements CommitValidationListener {
private final AtomicInteger count = new AtomicInteger();
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
count.incrementAndGet();
return Collections.emptyList();
}
public int count() {
return count.get();
}
}
@Test
public void skipValidation() throws Exception {
String master = "refs/heads/master";
TestValidator validator = new TestValidator();
RegistrationHandle handle = commitValidators.add("test-validator", validator);
try {
// Validation listener is called on normal push
PushOneCommit push =
pushFactory.create(db, admin.getIdent(), testRepo, "change1", "a.txt", "content");
PushOneCommit.Result r = push.to(master);
r.assertOkStatus();
assertThat(validator.count()).isEqualTo(1);
// Push is rejected and validation listener is not called when not allowed
// to use skip option
PushOneCommit push2 =
pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
push2.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION));
r = push2.to(master);
r.assertErrorStatus("not permitted: skip validation");
assertThat(validator.count()).isEqualTo(1);
// Validation listener is not called when skip option is used
grantSkipValidation(project, master, SystemGroupBackend.REGISTERED_USERS);
PushOneCommit push3 =
pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
push3.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION));
r = push3.to(master);
r.assertOkStatus();
assertThat(validator.count()).isEqualTo(1);
} finally {
handle.remove();
}
}
@Test
public void pushToPublishMagicBranchIsAllowed() throws Exception {
// Push to "refs/publish/*" will be a synonym of "refs/for/*".

10
polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.html
View File

@@ -16,12 +16,14 @@ limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
<link rel="import" href="../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.html">
<link rel="import" href="../../plugins/gr-endpoint-param/gr-endpoint-param.html">
<link rel="import" href="../../shared/gr-avatar/gr-avatar.html">
<link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../../styles/dashboard-header-styles.html">
<link rel="import" href="../../../styles/shared-styles.html">
<dom-module id="gr-user-header">
<template>
@@ -62,6 +64,12 @@ limitations under the License.
date-str="[[_computeDetail(_accountDetails, 'registered_on')]]">
</gr-date-formatter>
</div>
<gr-endpoint-decorator name="user-header">
<gr-endpoint-param name="accountDetails" value="[[_accountDetails]]">
</gr-endpoint-param>
<gr-endpoint-param name="loggedIn" value="[[loggedIn]]">
</gr-endpoint-param>
</gr-endpoint-decorator>
</div>
<div class="info">
<div class$="[[_computeDashboardLinkClass(showDashboardLink, loggedIn)]]">