Merge "Make consistent avatars in change log"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 47afdba..8509b1f 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1425,8 +1425,7 @@
any of their groups is used.
This limit applies not only to the link:cmd-query.html[`gerrit query`]
-command, but also to the web UI results pagination size in the new
-PolyGerrit UI and, limited to the full project list, in the old GWT UI.
+command, but also to the web UI results pagination size.
[[capability_readAs]]
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 8ad8669..1bf51a1 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -828,6 +828,49 @@
+
If 0 or negative, disk storage for the cache is disabled.
+[[cache.name.expireAfterWrite]]cache.<name>.expireAfterWrite::
++
+Duration after which a cached value will be evicted and not
+read anymore.
++
+Values should use common unit suffixes to express their setting:
++
+* ms, milliseconds
+* s, sec, second, seconds
+* m, min, minute, minutes
+* h, hr, hour, hours
++
+Disabled by default.
+
+[[cache.name.refreshAfterWrite]]cache.<name>.refreshAfterWrite::
++
+Duration after which we asynchronously refresh the cached value.
++
+Values should use common unit suffixes to express their setting:
++
+* ms, milliseconds
+* s, sec, second, seconds
+* m, min, minute, minutes
+* h, hr, hour, hours
++
+This applies only to these caches that support refreshing:
++
+* `"projects"`: Caching project information in-memory. Defaults to 15 minutes.
+
+[[cache.refreshThreadPoolSize]]cache.refreshThreadPoolSize::
++
+Number of threads that are available to refresh cached values that became
+out of date. This applies only to these caches that support refreshing:
++
+* `"projects"`: Caching project information in-memory
++
+Refreshes will only be scheduled on this executor if the values are
+out of sync.
+The check if they are is cheap and always happens on the thread that
+inquires for a cached value.
++
+Defaults to 2.
+
==== [[cache_names]]Standard Caches
cache `"accounts"`::
@@ -1125,22 +1168,6 @@
+
Default is true, enabled.
-[[cache.projects.checkFrequency]]cache.projects.checkFrequency::
-+
-How often project configuration should be checked for update from Git.
-Gerrit Code Review caches project access rules and configuration in
-memory, checking the refs/meta/config branch every checkFrequency
-minutes to see if a new revision should be loaded and used for future
-access. Values can be specified using standard time unit abbreviations
-('ms', 'sec', 'min', etc.).
-+
-If set to 0, checks occur every time, which may slow down operations.
-If set to 'disabled' or 'off', no check will ever be done.
-Administrators may force the cache to flush with
-link:cmd-flush-caches.html[gerrit flush-caches].
-+
-Default is 5 minutes.
-
[[cache.projects.loadOnStartup]]cache.projects.loadOnStartup::
+
If the project cache should be loaded during server startup.
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 7e6799be..3040348 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -64,6 +64,7 @@
* `caches/memory_eviction_count`: Memory eviction count.
* `caches/disk_cached`: Disk entries used by persistent cache.
* `caches/disk_hit_ratio`: Disk hit ratio for persistent cache.
+* `caches/refresh_count`: The number of refreshes per cache with an indicator if a reload was necessary.
=== Change
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 0ef6ad5..2d62608 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -483,7 +483,6 @@
cfg.setString("gerrit", null, "basePath", "git");
cfg.setBoolean("sendemail", null, "enable", true);
cfg.setInt("sendemail", null, "threadPoolSize", 0);
- cfg.setInt("cache", "projects", "checkFrequency", 0);
cfg.setInt("plugins", null, "checkFrequency", 0);
cfg.setInt("sshd", null, "threads", 1);
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index 05992d4..0c3b7b0 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -246,7 +246,7 @@
}
private boolean sshdOff() {
- return new SshAddressesModule().getListenAddresses(config).isEmpty();
+ return new SshAddressesModule().provideListenAddresses(config).isEmpty();
}
private Injector createCfgInjector() {
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 034e042e..57bec71 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -380,7 +380,7 @@
}
private boolean sshdOff() {
- return new SshAddressesModule().getListenAddresses(config).isEmpty();
+ return new SshAddressesModule().provideListenAddresses(config).isEmpty();
}
private String myVersion() {
diff --git a/java/com/google/gerrit/server/CacheRefreshExecutor.java b/java/com/google/gerrit/server/CacheRefreshExecutor.java
new file mode 100644
index 0000000..1a377c3
--- /dev/null
+++ b/java/com/google/gerrit/server/CacheRefreshExecutor.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2020 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;
+
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import com.google.inject.BindingAnnotation;
+import java.lang.annotation.Retention;
+
+/**
+ * Marker on the global {@link java.util.concurrent.ThreadPoolExecutor} used to refresh outdated
+ * values in caches.
+ */
+@Retention(RUNTIME)
+@BindingAnnotation
+public @interface CacheRefreshExecutor {}
diff --git a/java/com/google/gerrit/server/cache/CacheBinding.java b/java/com/google/gerrit/server/cache/CacheBinding.java
index 9d90d073..99db64e 100644
--- a/java/com/google/gerrit/server/cache/CacheBinding.java
+++ b/java/com/google/gerrit/server/cache/CacheBinding.java
@@ -29,6 +29,13 @@
/** Set the time an element lives after last access before being expired. */
CacheBinding<K, V> expireFromMemoryAfterAccess(Duration duration);
+ /**
+ * Set the time that an element will be refreshed after. Elements older than this but younger than
+ * {@link #expireAfterWrite(Duration)} will still be returned, but on access a task is queued to
+ * refresh their value asynchronously.
+ */
+ CacheBinding<K, V> refreshAfterWrite(Duration duration);
+
/** Populate the cache with items from the CacheLoader. */
CacheBinding<K, V> loader(Class<? extends CacheLoader<K, V>> clazz);
diff --git a/java/com/google/gerrit/server/cache/CacheDef.java b/java/com/google/gerrit/server/cache/CacheDef.java
index d0c633e..31a453e 100644
--- a/java/com/google/gerrit/server/cache/CacheDef.java
+++ b/java/com/google/gerrit/server/cache/CacheDef.java
@@ -51,6 +51,9 @@
Duration expireFromMemoryAfterAccess();
@Nullable
+ Duration refreshAfterWrite();
+
+ @Nullable
Weigher<K, V> weigher();
@Nullable
diff --git a/java/com/google/gerrit/server/cache/CacheProvider.java b/java/com/google/gerrit/server/cache/CacheProvider.java
index fe4244c..2dd9e1f 100644
--- a/java/com/google/gerrit/server/cache/CacheProvider.java
+++ b/java/com/google/gerrit/server/cache/CacheProvider.java
@@ -38,6 +38,7 @@
private long maximumWeight;
private Duration expireAfterWrite;
private Duration expireFromMemoryAfterAccess;
+ private Duration refreshAfterWrite;
private Provider<CacheLoader<K, V>> loader;
private Provider<Weigher<K, V>> weigher;
@@ -90,6 +91,13 @@
}
@Override
+ public CacheBinding<K, V> refreshAfterWrite(Duration duration) {
+ checkNotFrozen();
+ refreshAfterWrite = duration;
+ return this;
+ }
+
+ @Override
public CacheBinding<K, V> loader(Class<? extends CacheLoader<K, V>> impl) {
checkNotFrozen();
loader = module.bindCacheLoader(this, impl);
@@ -151,6 +159,11 @@
}
@Override
+ public Duration refreshAfterWrite() {
+ return refreshAfterWrite;
+ }
+
+ @Override
@Nullable
public Weigher<K, V> weigher() {
return weigher != null ? weigher.get() : null;
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheDefProxy.java b/java/com/google/gerrit/server/cache/h2/H2CacheDefProxy.java
index 5d9ce60..aa62745 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheDefProxy.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheDefProxy.java
@@ -43,6 +43,11 @@
}
@Override
+ public Duration refreshAfterWrite() {
+ return source.refreshAfterWrite();
+ }
+
+ @Override
public Weigher<K, V> weigher() {
Weigher<K, V> weigher = source.weigher();
if (weigher == null) {
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
index 8f7e360..82615a4 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
@@ -237,6 +237,7 @@
def.valueSerializer(),
def.version(),
maxSize,
- def.expireAfterWrite());
+ def.expireAfterWrite(),
+ def.expireFromMemoryAfterAccess());
}
}
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
index ef4e44c..7a53600 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
@@ -23,6 +23,9 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.BloomFilter;
+import com.google.common.util.concurrent.FutureCallback;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.cache.PersistentCache;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
@@ -40,6 +43,7 @@
import java.sql.Statement;
import java.sql.Timestamp;
import java.time.Duration;
+import java.time.Instant;
import java.util.Calendar;
import java.util.Map;
import java.util.concurrent.ArrayBlockingQueue;
@@ -122,7 +126,12 @@
@Override
public V get(K key) throws ExecutionException {
if (mem instanceof LoadingCache) {
- return ((LoadingCache<K, ValueHolder<V>>) mem).get(key).value;
+ LoadingCache<K, ValueHolder<V>> asLoadingCache = (LoadingCache<K, ValueHolder<V>>) mem;
+ ValueHolder<V> valueHolder = asLoadingCache.get(key);
+ if (store.needsRefresh(valueHolder.created)) {
+ asLoadingCache.refresh(key);
+ }
+ return valueHolder.value;
}
throw new UnsupportedOperationException();
}
@@ -139,8 +148,8 @@
}
}
- ValueHolder<V> h = new ValueHolder<>(valueLoader.call());
- h.created = TimeUtil.nowMs();
+ ValueHolder<V> h =
+ new ValueHolder<>(valueLoader.call(), Instant.ofEpochMilli(TimeUtil.nowMs()));
executor.execute(() -> store.put(key, h));
return h;
})
@@ -149,8 +158,7 @@
@Override
public void put(K key, V val) {
- final ValueHolder<V> h = new ValueHolder<>(val);
- h.created = TimeUtil.nowMs();
+ final ValueHolder<V> h = new ValueHolder<>(val, Instant.ofEpochMilli(TimeUtil.nowMs()));
mem.put(key, h);
executor.execute(() -> store.put(key, h));
}
@@ -217,11 +225,12 @@
static class ValueHolder<V> {
final V value;
- long created;
+ final Instant created;
volatile boolean clean;
- ValueHolder(V value) {
+ ValueHolder(V value, Instant created) {
this.value = value;
+ this.created = created;
}
}
@@ -248,12 +257,34 @@
}
}
- final ValueHolder<V> h = new ValueHolder<>(loader.load(key));
- h.created = TimeUtil.nowMs();
+ final ValueHolder<V> h =
+ new ValueHolder<>(loader.load(key), Instant.ofEpochMilli(TimeUtil.nowMs()));
executor.execute(() -> store.put(key, h));
return h;
}
}
+
+ @Override
+ public ListenableFuture<ValueHolder<V>> reload(K key, ValueHolder<V> oldValue)
+ throws Exception {
+ ListenableFuture<V> reloadedValue = loader.reload(key, oldValue.value);
+ Futures.addCallback(
+ reloadedValue,
+ new FutureCallback<V>() {
+ @Override
+ public void onSuccess(V result) {
+ store.put(key, new ValueHolder<>(result, TimeUtil.now()));
+ }
+
+ @Override
+ public void onFailure(Throwable t) {
+ logger.atWarning().withCause(t).log("Unable to reload cache value");
+ }
+ },
+ executor);
+
+ return Futures.transform(reloadedValue, v -> new ValueHolder<>(v, TimeUtil.now()), executor);
+ }
}
static class SqlStore<K, V> {
@@ -263,6 +294,7 @@
private final int version;
private final long maxSize;
@Nullable private final Duration expireAfterWrite;
+ @Nullable private final Duration refreshAfterWrite;
private final BlockingQueue<SqlHandle> handles;
private final AtomicLong hitCount = new AtomicLong();
private final AtomicLong missCount = new AtomicLong();
@@ -276,13 +308,15 @@
CacheSerializer<V> valueSerializer,
int version,
long maxSize,
- @Nullable Duration expireAfterWrite) {
+ @Nullable Duration expireAfterWrite,
+ @Nullable Duration refreshAfterWrite) {
this.url = jdbcUrl;
this.keyType = createKeyType(keyType, keySerializer);
this.valueSerializer = valueSerializer;
this.version = version;
this.maxSize = maxSize;
this.expireAfterWrite = expireAfterWrite;
+ this.refreshAfterWrite = refreshAfterWrite;
int cores = Runtime.getRuntime().availableProcessors();
int keep = Math.min(cores, 16);
@@ -394,14 +428,14 @@
}
Timestamp created = r.getTimestamp(2);
- if (expired(created)) {
+ if (expired(created.toInstant())) {
invalidate(key);
missCount.incrementAndGet();
return null;
}
V val = valueSerializer.deserialize(r.getBytes(1));
- ValueHolder<V> h = new ValueHolder<>(val);
+ ValueHolder<V> h = new ValueHolder<>(val, created.toInstant());
h.clean = true;
hitCount.incrementAndGet();
touch(c, key);
@@ -429,14 +463,22 @@
return false;
}
- private boolean expired(Timestamp created) {
+ private boolean expired(Instant created) {
if (expireAfterWrite == null) {
return false;
}
- Duration age = Duration.between(created.toInstant(), TimeUtil.now());
+ Duration age = Duration.between(created, TimeUtil.now());
return age.compareTo(expireAfterWrite) > 0;
}
+ private boolean needsRefresh(Instant created) {
+ if (refreshAfterWrite == null) {
+ return false;
+ }
+ Duration age = Duration.between(created, TimeUtil.now());
+ return age.compareTo(refreshAfterWrite) > 0;
+ }
+
private void touch(SqlHandle c, K key) throws IOException, SQLException {
if (c.touch == null) {
c.touch = c.conn.prepareStatement("UPDATE data SET accessed=? WHERE k=? AND version=?");
@@ -474,7 +516,7 @@
keyType.set(c.put, 1, key);
c.put.setBytes(2, valueSerializer.serialize(holder.value));
c.put.setInt(3, version);
- c.put.setTimestamp(4, new Timestamp(holder.created));
+ c.put.setTimestamp(4, Timestamp.from(holder.created));
c.put.setTimestamp(5, TimeUtil.nowTs());
c.put.executeUpdate();
holder.clean = true;
@@ -560,7 +602,7 @@
while (maxSize < used && r.next()) {
K key = keyType.get(r, 1);
Timestamp created = r.getTimestamp(3);
- if (mem.getIfPresent(key) != null && !expired(created)) {
+ if (mem.getIfPresent(key) != null && !expired(created.toInstant())) {
touch(c, key);
} else {
invalidate(c, key);
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 9906b3d..23caca7 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -105,6 +105,21 @@
builder.expireAfterAccess(expireAfterAccess.toNanos(), NANOSECONDS);
}
+ Duration refreshAfterWrite = def.refreshAfterWrite();
+ if (has(def.configKey(), "refreshAfterWrite")) {
+ builder.refreshAfterWrite(
+ ConfigUtil.getTimeUnit(
+ cfg,
+ "cache",
+ def.configKey(),
+ "refreshAfterWrite",
+ toSeconds(refreshAfterWrite),
+ SECONDS),
+ SECONDS);
+ } else if (refreshAfterWrite != null) {
+ builder.refreshAfterWrite(refreshAfterWrite.toNanos(), NANOSECONDS);
+ }
+
return builder;
}
@@ -141,6 +156,21 @@
builder.expireAfterAccess(expireAfterAccess.toNanos(), NANOSECONDS);
}
+ Duration refreshAfterWrite = def.refreshAfterWrite();
+ if (has(def.configKey(), "refreshAfterWrite")) {
+ builder.expireAfterAccess(
+ ConfigUtil.getTimeUnit(
+ cfg,
+ "cache",
+ def.configKey(),
+ "refreshAfterWrite",
+ toSeconds(refreshAfterWrite),
+ SECONDS),
+ SECONDS);
+ } else if (refreshAfterWrite != null) {
+ builder.refreshAfterWrite(refreshAfterWrite.toNanos(), NANOSECONDS);
+ }
+
return builder;
}
diff --git a/java/com/google/gerrit/server/config/SysExecutorModule.java b/java/com/google/gerrit/server/config/SysExecutorModule.java
index e7f4540..ea45b12 100644
--- a/java/com/google/gerrit/server/config/SysExecutorModule.java
+++ b/java/com/google/gerrit/server/config/SysExecutorModule.java
@@ -14,7 +14,11 @@
package com.google.gerrit.server.config;
+import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService;
+
+import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
+import com.google.gerrit.server.CacheRefreshExecutor;
import com.google.gerrit.server.FanOutExecutor;
import com.google.gerrit.server.git.WorkQueue;
import com.google.inject.AbstractModule;
@@ -24,7 +28,7 @@
import org.eclipse.jgit.lib.Config;
/**
- * Module providing the {@link ReceiveCommitsExecutor}.
+ * Module providing different executors.
*
* <p>This module is intended to be installed at the top level when creating a {@code sysInjector}
* in {@code Daemon} or similar, not nested in another module. This ensures the module can be
@@ -37,7 +41,7 @@
@Provides
@Singleton
@ReceiveCommitsExecutor
- public ExecutorService createReceiveCommitsExecutor(
+ public ExecutorService provideReceiveCommitsExecutor(
@GerritServerConfig Config config, WorkQueue queues) {
int poolSize =
config.getInt(
@@ -48,11 +52,11 @@
@Provides
@Singleton
@SendEmailExecutor
- public ExecutorService createSendEmailExecutor(
+ public ExecutorService provideSendEmailExecutor(
@GerritServerConfig Config config, WorkQueue queues) {
int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1);
if (poolSize == 0) {
- return MoreExecutors.newDirectExecutorService();
+ return newDirectExecutorService();
}
return queues.createQueue(poolSize, "SendEmail", true);
}
@@ -60,11 +64,24 @@
@Provides
@Singleton
@FanOutExecutor
- public ExecutorService createFanOutExecutor(@GerritServerConfig Config config, WorkQueue queues) {
+ public ExecutorService provideFanOutExecutor(
+ @GerritServerConfig Config config, WorkQueue queues) {
int poolSize = config.getInt("execution", null, "fanOutThreadPoolSize", 25);
if (poolSize == 0) {
- return MoreExecutors.newDirectExecutorService();
+ return newDirectExecutorService();
}
return queues.createQueue(poolSize, "FanOut");
}
+
+ @Provides
+ @Singleton
+ @CacheRefreshExecutor
+ public ListeningExecutorService provideCacheRefreshExecutor(
+ @GerritServerConfig Config config, WorkQueue queues) {
+ int poolSize = config.getInt("cache", null, "refreshThreadPoolSize", 2);
+ if (poolSize == 0) {
+ return newDirectExecutorService();
+ }
+ return MoreExecutors.listeningDecorator(queues.createQueue(poolSize, "CacheRefresh"));
+ }
}
diff --git a/java/com/google/gerrit/server/config/UrlFormatter.java b/java/com/google/gerrit/server/config/UrlFormatter.java
index 8fc0d9e..d3f90e5 100644
--- a/java/com/google/gerrit/server/config/UrlFormatter.java
+++ b/java/com/google/gerrit/server/config/UrlFormatter.java
@@ -52,6 +52,11 @@
return getChangeViewUrl(change.getProject(), change.getId()).map(url -> url + "?tab=comments");
}
+ /** Returns the URL for viewing the findings tab view of a change. */
+ default Optional<String> getFindingsTabView(Change change) {
+ return getChangeViewUrl(change.getProject(), change.getId()).map(url -> url + "?tab=findings");
+ }
+
/** Returns the URL for viewing a file in a given patch set of a change. */
default Optional<String> getPatchFileView(Change change, int patchsetId, String filename) {
return getChangeViewUrl(change.getProject(), change.getId())
diff --git a/java/com/google/gerrit/server/logging/Metadata.java b/java/com/google/gerrit/server/logging/Metadata.java
index 3bb4770..0a99d83 100644
--- a/java/com/google/gerrit/server/logging/Metadata.java
+++ b/java/com/google/gerrit/server/logging/Metadata.java
@@ -31,116 +31,123 @@
/** Metadata that is provided to {@link PerformanceLogger}s as context for performance records. */
@AutoValue
public abstract class Metadata {
- // The numeric ID of an account.
+ /** The numeric ID of an account. */
public abstract Optional<Integer> accountId();
- // The type of an action (ACCOUNT_UPDATE, CHANGE_UPDATE, GROUP_UPDATE, INDEX_QUERY,
- // PLUGIN_UPDATE).
+ /**
+ * The type of an action (ACCOUNT_UPDATE, CHANGE_UPDATE, GROUP_UPDATE, INDEX_QUERY,
+ * PLUGIN_UPDATE).
+ */
public abstract Optional<String> actionType();
- // An authentication domain name.
+ /** An authentication domain name. */
public abstract Optional<String> authDomainName();
- // The name of a branch.
+ /** The name of a branch. */
public abstract Optional<String> branchName();
- // Key of an entity in a cache.
+ /** Key of an entity in a cache. */
public abstract Optional<String> cacheKey();
- // The name of a cache.
+ /** The name of a cache. */
public abstract Optional<String> cacheName();
- // The name of the implementation class.
+ /** The name of the implementation class. */
public abstract Optional<String> className();
- // The numeric ID of a change.
+ /** The numeric ID of a change. */
public abstract Optional<Integer> changeId();
- // The type of change ID which the user used to identify a change (e.g. numeric ID, triplet etc.).
+ /**
+ * The type of change ID which the user used to identify a change (e.g. numeric ID, triplet etc.).
+ */
public abstract Optional<String> changeIdType();
- // The cause of an error.
+ /** The cause of an error. */
public abstract Optional<String> cause();
- // The type of an event.
+ /** The type of an event. */
public abstract Optional<String> eventType();
- // The value of the @Export annotation which was used to register a plugin extension.
+ /** The value of the @Export annotation which was used to register a plugin extension. */
public abstract Optional<String> exportValue();
- // Path of a file in a repository.
+ /** Path of a file in a repository. */
public abstract Optional<String> filePath();
- // Garbage collector name.
+ /** Garbage collector name. */
public abstract Optional<String> garbageCollectorName();
- // Git operation (CLONE, FETCH).
+ /** Git operation (CLONE, FETCH). */
public abstract Optional<String> gitOperation();
- // The numeric ID of an internal group.
+ /** The numeric ID of an internal group. */
public abstract Optional<Integer> groupId();
- // The name of a group.
+ /** The name of a group. */
public abstract Optional<String> groupName();
- // The UUID of a group.
+ /** The UUID of a group. */
public abstract Optional<String> groupUuid();
- // HTTP status response code.
+ /** HTTP status response code. */
public abstract Optional<Integer> httpStatus();
- // The name of a secondary index.
+ /** The name of a secondary index. */
public abstract Optional<String> indexName();
- // The version of a secondary index.
+ /** The version of a secondary index. */
public abstract Optional<Integer> indexVersion();
- // The name of the implementation method.
+ /** The name of the implementation method. */
public abstract Optional<String> methodName();
- // One or more resources
+ /** One or more resources */
public abstract Optional<Boolean> multiple();
- // The name of an operation that is performed.
+ /** The name of an operation that is performed. */
public abstract Optional<String> operationName();
- // Partial or full computation
+ /** Partial or full computation */
public abstract Optional<Boolean> partial();
- // Path of a metadata file in NoteDb.
+ /** If a value is still current or not */
+ public abstract Optional<Boolean> outdated();
+
+ /** Path of a metadata file in NoteDb. */
public abstract Optional<String> noteDbFilePath();
- // Name of a metadata ref in NoteDb.
+ /** Name of a metadata ref in NoteDb. */
public abstract Optional<String> noteDbRefName();
- // Type of a sequence in NoteDb (ACCOUNTS, CHANGES, GROUPS).
+ /** Type of a sequence in NoteDb (ACCOUNTS, CHANGES, GROUPS). */
public abstract Optional<String> noteDbSequenceType();
- // The ID of a patch set.
+ /** The ID of a patch set. */
public abstract Optional<Integer> patchSetId();
- // Plugin metadata that doesn't fit into any other category.
+ /** Plugin metadata that doesn't fit into any other category. */
public abstract ImmutableList<PluginMetadata> pluginMetadata();
- // The name of a plugin.
+ /** The name of a plugin. */
public abstract Optional<String> pluginName();
- // The name of a Gerrit project (aka Git repository).
+ /** The name of a Gerrit project (aka Git repository). */
public abstract Optional<String> projectName();
- // The type of a Git push to Gerrit (CREATE_REPLACE, NORMAL, AUTOCLOSE).
+ /** The type of a Git push to Gerrit (CREATE_REPLACE, NORMAL, AUTOCLOSE). */
public abstract Optional<String> pushType();
- // The number of resources that is processed.
+ /** The number of resources that is processed. */
public abstract Optional<Integer> resourceCount();
- // The name of a REST view.
+ /** The name of a REST view. */
public abstract Optional<String> restViewName();
- // The SHA1 of Git commit.
+ /** The SHA1 of Git commit. */
public abstract Optional<String> revision();
- // The username of an account.
+ /** The username of an account. */
public abstract Optional<String> username();
/**
@@ -305,6 +312,8 @@
public abstract Builder partial(boolean partial);
+ public abstract Builder outdated(boolean outdated);
+
public abstract Builder noteDbFilePath(@Nullable String noteDbFilePath);
public abstract Builder noteDbRefName(@Nullable String noteDbRefName);
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java
index deaa926..f3cccf2 100644
--- a/java/com/google/gerrit/server/mail/send/CommentSender.java
+++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -93,6 +93,11 @@
return args.urlFormatter.get().getCommentsTabView(change).orElse(null);
}
+ /** @return a web link to the findings tab view of a change. */
+ public String getFindingsTabLink() {
+ return args.urlFormatter.get().getFindingsTabView(change).orElse(null);
+ }
+
/**
* @return A title for the group, i.e. "Commit Message", "Merge List", or "File [[filename]]".
*/
@@ -386,9 +391,7 @@
for (CommentSender.FileCommentGroup group : getGroupedInlineComments(repo)) {
Map<String, Object> groupData = new HashMap<>();
- if (group.filename.equals(Patch.PATCHSET_LEVEL)) {
- groupData.put("link", group.getCommentsTabLink());
- } else {
+ if (!group.filename.equals(Patch.PATCHSET_LEVEL)) {
groupData.put("link", group.getFileLink());
}
groupData.put("title", group.getTitle());
@@ -420,7 +423,11 @@
// Set the comment link.
if (comment.key.filename.equals(Patch.PATCHSET_LEVEL)) {
- commentData.put("link", group.getCommentsTabLink());
+ if (comment instanceof RobotComment) {
+ commentData.put("link", group.getFindingsTabLink());
+ } else {
+ commentData.put("link", group.getCommentsTabLink());
+ }
} else if (comment.lineNbr == 0) {
commentData.put("link", group.getFileLink());
} else {
diff --git a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
index 128e185..1ead03c 100644
--- a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
+++ b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java
@@ -27,6 +27,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.function.Consumer;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -47,12 +48,16 @@
private static final String EMPTY_TREE_ID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";
private static final String DRAFT_REFS_PREFIX = "refs/draft-comments";
- private static final int CHUNK_SIZE = 100; // log progress after deleting every CHUNK_SIZE refs
+
+ // Number of refs deleted at once in a batch ref-update.
+ // Log progress after deleting every CHUNK_SIZE refs
+ private static final int CHUNK_SIZE = 3000;
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final int cleanupPercentage;
private Repository allUsersRepo;
+ private final Consumer<String> uiConsumer;
public interface Factory {
DeleteZombieCommentsRefs create(int cleanupPercentage);
@@ -63,9 +68,18 @@
AllUsersName allUsers,
GitRepositoryManager repoManager,
@Assisted Integer cleanupPercentage) {
+ this(allUsers, repoManager, cleanupPercentage, (msg) -> {});
+ }
+
+ public DeleteZombieCommentsRefs(
+ AllUsersName allUsers,
+ GitRepositoryManager repoManager,
+ Integer cleanupPercentage,
+ Consumer<String> uiConsumer) {
this.allUsers = allUsers;
this.repoManager = repoManager;
this.cleanupPercentage = (cleanupPercentage == null) ? 100 : cleanupPercentage;
+ this.uiConsumer = uiConsumer;
}
public void execute() throws IOException {
@@ -74,15 +88,17 @@
List<Ref> draftRefs = allUsersRepo.getRefDatabase().getRefsByPrefix(DRAFT_REFS_PREFIX);
List<Ref> zombieRefs = filterZombieRefs(draftRefs);
- logger.atInfo().log(
- "Found a total of %d zombie draft refs in %s repo.", zombieRefs.size(), allUsers.get());
+ logInfo(
+ String.format(
+ "Found a total of %d zombie draft refs in %s repo.",
+ zombieRefs.size(), allUsers.get()));
- logger.atInfo().log("Cleanup percentage = %d", cleanupPercentage);
+ logInfo(String.format("Cleanup percentage = %d", cleanupPercentage));
zombieRefs =
zombieRefs.stream()
.filter(ref -> Change.Id.fromAllUsersRef(ref.getName()).get() % 100 < cleanupPercentage)
.collect(toImmutableList());
- logger.atInfo().log("Number of zombie refs to be cleaned = %d", zombieRefs.size());
+ logInfo(String.format("Number of zombie refs to be cleaned = %d", zombieRefs.size()));
long zombieRefsCnt = zombieRefs.size();
long deletedRefsCnt = 0;
@@ -124,8 +140,15 @@
return allUsersRepo.parseCommit(ref.getObjectId()).getTree().getName().equals(EMPTY_TREE_ID);
}
+ private void logInfo(String message) {
+ logger.atInfo().log(message);
+ uiConsumer.accept(message);
+ }
+
private void logProgress(long deletedRefsCount, long allRefsCount, long elapsed) {
- logger.atInfo().log(
- "Deleted %d/%d zombie draft refs (%d seconds)\n", deletedRefsCount, allRefsCount, elapsed);
+ logInfo(
+ String.format(
+ "Deleted %d/%d zombie draft refs (%d seconds)",
+ deletedRefsCount, allRefsCount, elapsed));
}
}
diff --git a/java/com/google/gerrit/server/patch/DiffExecutorModule.java b/java/com/google/gerrit/server/patch/DiffExecutorModule.java
index eb6a280..b9e644f 100644
--- a/java/com/google/gerrit/server/patch/DiffExecutorModule.java
+++ b/java/com/google/gerrit/server/patch/DiffExecutorModule.java
@@ -31,7 +31,7 @@
@Provides
@Singleton
@DiffExecutor
- public ExecutorService createDiffExecutor() {
+ public ExecutorService provideDiffExecutor() {
return new LoggingContextAwareExecutorService(
Executors.newCachedThreadPool(
new ThreadFactoryBuilder().setNameFormat("Diff-%d").setDaemon(true).build()));
diff --git a/java/com/google/gerrit/server/project/ProjectCacheClock.java b/java/com/google/gerrit/server/project/ProjectCacheClock.java
deleted file mode 100644
index eb451fd..0000000
--- a/java/com/google/gerrit/server/project/ProjectCacheClock.java
+++ /dev/null
@@ -1,100 +0,0 @@
-// Copyright (C) 2012 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.project;
-
-import com.google.common.util.concurrent.ThreadFactoryBuilder;
-import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.logging.LoggingContextAwareScheduledExecutorService;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import java.util.concurrent.Executors;
-import java.util.concurrent.Future;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
-import org.eclipse.jgit.lib.Config;
-
-/** Ticks periodically to force refresh events for {@link ProjectCacheImpl}. */
-@Singleton
-public class ProjectCacheClock implements LifecycleListener {
- private final Config serverConfig;
-
- private final AtomicLong generation = new AtomicLong();
-
- private ScheduledExecutorService executor;
-
- @Inject
- public ProjectCacheClock(@GerritServerConfig Config serverConfig) {
- this.serverConfig = serverConfig;
- }
-
- @Override
- public void start() {
- long checkFrequencyMillis = checkFrequency(serverConfig);
-
- if (checkFrequencyMillis == Long.MAX_VALUE) {
- // Start with generation 1 (to avoid magic 0 below).
- // Do not begin background thread, disabling the clock.
- generation.set(1);
- } else if (10 < checkFrequencyMillis) {
- // Start with generation 1 (to avoid magic 0 below).
- generation.set(1);
- executor =
- new LoggingContextAwareScheduledExecutorService(
- Executors.newScheduledThreadPool(
- 1,
- new ThreadFactoryBuilder()
- .setNameFormat("ProjectCacheClock-%d")
- .setDaemon(true)
- .setPriority(Thread.MIN_PRIORITY)
- .build()));
- @SuppressWarnings("unused") // Runnable already handles errors
- Future<?> possiblyIgnoredError =
- executor.scheduleAtFixedRate(
- generation::incrementAndGet,
- checkFrequencyMillis,
- checkFrequencyMillis,
- TimeUnit.MILLISECONDS);
- } else {
- // Magic generation 0 triggers ProjectState to always
- // check on each needsRefresh() request we make to it.
- generation.set(0);
- }
- }
-
- @Override
- public void stop() {
- if (executor != null) {
- executor.shutdown();
- }
- }
-
- long read() {
- return generation.get();
- }
-
- private static long checkFrequency(Config serverConfig) {
- String freq = serverConfig.getString("cache", "projects", "checkFrequency");
- if (freq != null && ("disabled".equalsIgnoreCase(freq) || "off".equalsIgnoreCase(freq))) {
- return Long.MAX_VALUE;
- }
- return TimeUnit.MILLISECONDS.convert(
- ConfigUtil.getTimeUnit(
- serverConfig, "cache", "projects", "checkFrequency", 5, TimeUnit.MINUTES),
- TimeUnit.MINUTES);
- }
-}
diff --git a/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
index 9d09eeb..a5c07e5 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -24,16 +24,23 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.project.ProjectIndexer;
import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.metrics.Counter2;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
+import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.server.CacheRefreshExecutor;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
@@ -48,6 +55,7 @@
import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.io.IOException;
+import java.time.Duration;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
@@ -55,6 +63,7 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
/** Cache of project information, including access rights. */
@@ -70,7 +79,10 @@
return new CacheModule() {
@Override
protected void configure() {
- cache(CACHE_NAME, String.class, ProjectState.class).loader(Loader.class);
+ cache(CACHE_NAME, Project.NameKey.class, ProjectState.class)
+ .loader(Loader.class)
+ .refreshAfterWrite(Duration.ofMinutes(15))
+ .expireAfterWrite(Duration.ofHours(1));
cache(CACHE_LIST, ListKey.class, new TypeLiteral<ImmutableSortedSet<Project.NameKey>>() {})
.maximumWeight(1)
@@ -84,7 +96,6 @@
@Override
protected void configure() {
listener().to(ProjectCacheWarmer.class);
- listener().to(ProjectCacheClock.class);
}
});
}
@@ -93,10 +104,9 @@
private final AllProjectsName allProjectsName;
private final AllUsersName allUsersName;
- private final LoadingCache<String, ProjectState> byName;
+ private final LoadingCache<Project.NameKey, ProjectState> byName;
private final LoadingCache<ListKey, ImmutableSortedSet<Project.NameKey>> list;
private final Lock listLock;
- private final ProjectCacheClock clock;
private final Provider<ProjectIndexer> indexer;
private final Timer0 guessRelevantGroupsLatency;
@@ -104,9 +114,8 @@
ProjectCacheImpl(
final AllProjectsName allProjectsName,
final AllUsersName allUsersName,
- @Named(CACHE_NAME) LoadingCache<String, ProjectState> byName,
+ @Named(CACHE_NAME) LoadingCache<Project.NameKey, ProjectState> byName,
@Named(CACHE_LIST) LoadingCache<ListKey, ImmutableSortedSet<Project.NameKey>> list,
- ProjectCacheClock clock,
Provider<ProjectIndexer> indexer,
MetricMaker metricMaker) {
this.allProjectsName = allProjectsName;
@@ -114,7 +123,6 @@
this.byName = byName;
this.list = list;
this.listLock = new ReentrantLock(true /* fair */);
- this.clock = clock;
this.indexer = indexer;
this.guessRelevantGroupsLatency =
@@ -142,13 +150,8 @@
}
try {
- ProjectState state = byName.get(projectName.get());
- if (state != null && state.needsRefresh(clock.read())) {
- byName.invalidate(projectName.get());
- state = byName.get(projectName.get());
- }
- return Optional.of(state);
- } catch (Exception e) {
+ return Optional.of(byName.get(projectName));
+ } catch (ExecutionException e) {
if ((e.getCause() instanceof RepositoryNotFoundException)) {
logger.atFine().log("Cannot find project %s", projectName.get());
return Optional.empty();
@@ -167,7 +170,7 @@
public void evict(Project.NameKey p) {
if (p != null) {
logger.atFine().log("Evict project '%s'", p.get());
- byName.invalidate(p.get());
+ byName.invalidate(p);
}
indexer.get().index(p);
}
@@ -222,7 +225,7 @@
public Set<AccountGroup.UUID> guessRelevantGroupUUIDs() {
try (Timer0.Context ignored = guessRelevantGroupsLatency.start()) {
return all().stream()
- .map(n -> byName.getIfPresent(n.get()))
+ .map(n -> byName.getIfPresent(n))
.filter(Objects::nonNull)
.flatMap(p -> p.getConfig().getAllGroupUUIDs().stream())
// getAllGroupUUIDs shouldn't really return null UUIDs, but harden
@@ -245,41 +248,67 @@
}
}
- static class Loader extends CacheLoader<String, ProjectState> {
+ @Singleton
+ static class Loader extends CacheLoader<Project.NameKey, ProjectState> {
private final ProjectState.Factory projectStateFactory;
private final GitRepositoryManager mgr;
- private final ProjectCacheClock clock;
private final ProjectConfig.Factory projectConfigFactory;
+ private final ListeningExecutorService cacheRefreshExecutor;
+ private final Counter2<String, Boolean> refreshCounter;
@Inject
Loader(
ProjectState.Factory psf,
GitRepositoryManager g,
- ProjectCacheClock clock,
- ProjectConfig.Factory projectConfigFactory) {
+ ProjectConfig.Factory projectConfigFactory,
+ @CacheRefreshExecutor ListeningExecutorService cacheRefreshExecutor,
+ MetricMaker metricMaker) {
projectStateFactory = psf;
mgr = g;
- this.clock = clock;
this.projectConfigFactory = projectConfigFactory;
+ this.cacheRefreshExecutor = cacheRefreshExecutor;
+ refreshCounter =
+ metricMaker.newCounter(
+ "caches/refresh_count",
+ new Description("count").setRate(),
+ Field.ofString("cache", Metadata.Builder::className).build(),
+ Field.ofBoolean("outdated", Metadata.Builder::outdated).build());
}
@Override
- public ProjectState load(String projectName) throws Exception {
+ public ProjectState load(Project.NameKey key) throws Exception {
try (TraceTimer timer =
TraceContext.newTimer(
- "Loading project", Metadata.builder().projectName(projectName).build())) {
- long now = clock.read();
- Project.NameKey key = Project.nameKey(projectName);
+ "Loading project", Metadata.builder().projectName(key.get()).build())) {
try (Repository git = mgr.openRepository(key)) {
ProjectConfig cfg = projectConfigFactory.create(key);
cfg.load(key, git);
-
- ProjectState state = projectStateFactory.create(cfg);
- state.initLastCheck(now);
- return state;
+ return projectStateFactory.create(cfg);
}
}
}
+
+ @Override
+ public ListenableFuture<ProjectState> reload(Project.NameKey key, ProjectState oldState)
+ throws Exception {
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ "Reload project", Metadata.builder().projectName(key.get()).build())) {
+ try (Repository git = mgr.openRepository(key)) {
+ Ref configRef = git.exactRef(RefNames.REFS_CONFIG);
+ if (configRef != null
+ && configRef.getObjectId().equals(oldState.getConfig().getRevision())) {
+ refreshCounter.increment(CACHE_NAME, false);
+ return Futures.immediateFuture(oldState);
+ }
+ }
+
+ // Repository is not thread safe, so we have to open it on the thread that does the loading.
+ // Just invoke the loader on the other thread.
+ refreshCounter.increment(CACHE_NAME, true);
+ return cacheRefreshExecutor.submit(() -> load(key));
+ }
+ }
}
static class ListKey {
diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java
index e52f344..efadcc8 100644
--- a/java/com/google/gerrit/server/project/ProjectState.java
+++ b/java/com/google/gerrit/server/project/ProjectState.java
@@ -32,7 +32,6 @@
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.projects.CommentLinkInfo;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -59,7 +58,6 @@
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
/**
@@ -86,9 +84,6 @@
private final long globalMaxObjectSizeLimit;
private final boolean inheritProjectMaxObjectSizeLimit;
- /** Last system time the configuration's revision was examined. */
- private volatile long lastCheckGeneration;
-
/** Local access sections, wrapped in SectionMatchers for faster evaluation. */
private volatile List<SectionMatcher> localAccessSections;
@@ -140,33 +135,6 @@
}
}
- void initLastCheck(long generation) {
- lastCheckGeneration = generation;
- }
-
- boolean needsRefresh(long generation) {
- if (generation <= 0) {
- return isRevisionOutOfDate();
- }
- if (lastCheckGeneration != generation) {
- lastCheckGeneration = generation;
- return isRevisionOutOfDate();
- }
- return false;
- }
-
- private boolean isRevisionOutOfDate() {
- try (Repository git = gitMgr.openRepository(getNameKey())) {
- Ref ref = git.getRefDatabase().exactRef(RefNames.REFS_CONFIG);
- if (ref == null || ref.getObjectId() == null) {
- return true;
- }
- return !ref.getObjectId().equals(config.getRevision());
- } catch (IOException gone) {
- return true;
- }
- }
-
/**
* @return cached computation of all global capabilities. This should only be invoked on the state
* from {@link ProjectCache#getAllProjects()}. Null on any other project.
diff --git a/java/com/google/gerrit/server/schema/Schema_182.java b/java/com/google/gerrit/server/schema/Schema_182.java
index 3928b2b..a61a175 100644
--- a/java/com/google/gerrit/server/schema/Schema_182.java
+++ b/java/com/google/gerrit/server/schema/Schema_182.java
@@ -28,7 +28,8 @@
public void upgrade(Arguments args, UpdateUI ui) throws Exception {
AllUsersName allUsers = args.allUsers;
GitRepositoryManager gitRepoManager = args.repoManager;
- DeleteZombieCommentsRefs cleanup = new DeleteZombieCommentsRefs(allUsers, gitRepoManager, 100);
+ DeleteZombieCommentsRefs cleanup =
+ new DeleteZombieCommentsRefs(allUsers, gitRepoManager, 100, ui::message);
cleanup.execute();
}
}
diff --git a/java/com/google/gerrit/server/ssh/SshAddressesModule.java b/java/com/google/gerrit/server/ssh/SshAddressesModule.java
index 0a6bcac..1159e06 100644
--- a/java/com/google/gerrit/server/ssh/SshAddressesModule.java
+++ b/java/com/google/gerrit/server/ssh/SshAddressesModule.java
@@ -40,7 +40,7 @@
@Provides
@Singleton
@SshListenAddresses
- public List<SocketAddress> getListenAddresses(@GerritServerConfig Config cfg) {
+ public List<SocketAddress> provideListenAddresses(@GerritServerConfig Config cfg) {
List<SocketAddress> listen = Lists.newArrayListWithExpectedSize(2);
String[] want = cfg.getStringList("sshd", null, "listenaddress");
if (want == null || want.length == 0) {
@@ -71,7 +71,7 @@
@Provides
@Singleton
@SshAdvertisedAddresses
- List<String> getAdvertisedAddresses(
+ List<String> provideAdvertisedAddresses(
@GerritServerConfig Config cfg, @SshListenAddresses List<SocketAddress> listen) {
String[] want = cfg.getStringList("sshd", null, "advertisedaddress");
if (want.length > 0) {
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 8800463..a682d33 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -15,10 +15,11 @@
package com.google.gerrit.testing;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService;
import static com.google.inject.Scopes.SINGLETON;
import com.google.common.base.Strings;
-import com.google.common.util.concurrent.MoreExecutors;
+import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl;
import com.google.gerrit.extensions.client.AuthType;
@@ -30,6 +31,7 @@
import com.google.gerrit.index.project.ProjectSchemaDefinitions;
import com.google.gerrit.metrics.DisabledMetricMaker;
import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.server.CacheRefreshExecutor;
import com.google.gerrit.server.FanOutExecutor;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.GerritPersonIdentProvider;
@@ -209,7 +211,7 @@
@Singleton
@DiffExecutor
public ExecutorService createDiffExecutor() {
- return MoreExecutors.newDirectExecutorService();
+ return newDirectExecutorService();
}
});
install(new DefaultMemoryCacheModule());
@@ -277,7 +279,7 @@
@Singleton
@SendEmailExecutor
public ExecutorService createSendEmailExecutor() {
- return MoreExecutors.newDirectExecutorService();
+ return newDirectExecutorService();
}
@Provides
@@ -287,6 +289,13 @@
return queues.createQueue(2, "FanOut");
}
+ @Provides
+ @Singleton
+ @CacheRefreshExecutor
+ public ListeningExecutorService createCacheRefreshExecutor() {
+ return newDirectExecutorService();
+ }
+
private Module luceneIndexModule() {
return indexModule("com.google.gerrit.lucene.LuceneIndexModule");
}
diff --git a/javatests/com/google/gerrit/server/cache/h2/BUILD b/javatests/com/google/gerrit/server/cache/h2/BUILD
index 98f1b0e..438990c 100644
--- a/javatests/com/google/gerrit/server/cache/h2/BUILD
+++ b/javatests/com/google/gerrit/server/cache/h2/BUILD
@@ -6,10 +6,12 @@
deps = [
"//java/com/google/gerrit/server/cache/h2",
"//java/com/google/gerrit/server/cache/serialize",
+ "//java/com/google/gerrit/server/util/time",
"//lib:guava",
"//lib:h2",
"//lib:junit",
"//lib/guice",
+ "//lib/mockito",
"//lib/truth",
],
)
diff --git a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
index 69c2799..fb3a0db 100644
--- a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
@@ -16,16 +16,27 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.server.cache.h2.H2CacheImpl.SqlStore;
import com.google.gerrit.server.cache.h2.H2CacheImpl.ValueHolder;
import com.google.gerrit.server.cache.serialize.StringCacheSerializer;
+import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.TypeLiteral;
+import java.time.Duration;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
import org.junit.Test;
public class H2CacheTest {
@@ -38,23 +49,31 @@
}
private static H2CacheImpl<String, String> newH2CacheImpl(
- int id, Cache<String, ValueHolder<String>> mem, int version) {
- SqlStore<String, String> store =
- new SqlStore<>(
- "jdbc:h2:mem:Test_" + id,
- KEY_TYPE,
- StringCacheSerializer.INSTANCE,
- StringCacheSerializer.INSTANCE,
- version,
- 1 << 20,
- null);
+ SqlStore<String, String> store, Cache<String, ValueHolder<String>> mem) {
return new H2CacheImpl<>(MoreExecutors.directExecutor(), store, KEY_TYPE, mem);
}
+ private static SqlStore<String, String> newStore(
+ int id,
+ int version,
+ @Nullable Duration expireAfterWrite,
+ @Nullable Duration refreshAfterWrite) {
+ return new SqlStore<>(
+ "jdbc:h2:mem:Test_" + id,
+ KEY_TYPE,
+ StringCacheSerializer.INSTANCE,
+ StringCacheSerializer.INSTANCE,
+ version,
+ 1 << 20,
+ expireAfterWrite,
+ refreshAfterWrite);
+ }
+
@Test
public void get() throws ExecutionException {
Cache<String, ValueHolder<String>> mem = CacheBuilder.newBuilder().build();
- H2CacheImpl<String, String> impl = newH2CacheImpl(nextDbId(), mem, DEFAULT_VERSION);
+ H2CacheImpl<String, String> impl =
+ newH2CacheImpl(newStore(nextDbId(), DEFAULT_VERSION, null, null), mem);
assertThat(impl.getIfPresent("foo")).isNull();
@@ -94,11 +113,12 @@
}
@Test
- public void version() throws Exception {
+ public void version() {
int id = nextDbId();
- H2CacheImpl<String, String> oldImpl = newH2CacheImpl(id, disableMemCache(), DEFAULT_VERSION);
+ H2CacheImpl<String, String> oldImpl =
+ newH2CacheImpl(newStore(id, DEFAULT_VERSION, null, null), disableMemCache());
H2CacheImpl<String, String> newImpl =
- newH2CacheImpl(id, disableMemCache(), DEFAULT_VERSION + 1);
+ newH2CacheImpl(newStore(id, DEFAULT_VERSION + 1, null, null), disableMemCache());
assertThat(oldImpl.diskStats().space()).isEqualTo(0);
assertThat(newImpl.diskStats().space()).isEqualTo(0);
@@ -124,6 +144,57 @@
assertThat(oldImpl.getIfPresent("key")).isNull();
}
+ @Test
+ public void refreshAfterWrite_triggeredWhenConfigured() throws Exception {
+ SqlStore<String, String> store =
+ newStore(nextDbId(), DEFAULT_VERSION, null, Duration.ofMillis(10));
+
+ // This is the loader that we configure for the cache when calling .loader(...)
+ @SuppressWarnings("unchecked")
+ CacheLoader<String, String> baseLoader = (CacheLoader<String, String>) mock(CacheLoader.class);
+ resetLoaderAndAnswerLoadAndRefreshCalls(baseLoader);
+
+ // We wrap baseLoader just like H2CacheFactory is wrapping it. The wrapped version will call out
+ // to the store for refreshing values.
+ H2CacheImpl.Loader<String, String> wrappedLoader =
+ new H2CacheImpl.Loader<>(MoreExecutors.directExecutor(), store, baseLoader);
+ // memCache is the in-memory variant of the cache. Its loader is wrappedLoader which will call
+ // out to the store to save or delete cached values.
+ LoadingCache<String, ValueHolder<String>> memCache =
+ CacheBuilder.newBuilder().maximumSize(10).build(wrappedLoader);
+
+ // h2Cache puts it all together
+ H2CacheImpl<String, String> h2Cache = newH2CacheImpl(store, memCache);
+
+ // Initial load and cache retrieval do not trigger refresh
+ // This works because we use a directExecutor() for refreshes
+ TimeUtil.setCurrentMillisSupplier(() -> 0);
+ assertThat(h2Cache.get("foo")).isEqualTo("load:foo");
+ verify(baseLoader).load("foo");
+ assertThat(h2Cache.get("foo")).isEqualTo("load:foo");
+ verifyNoMoreInteractions(baseLoader);
+ resetLoaderAndAnswerLoadAndRefreshCalls(baseLoader);
+
+ // Load after refresh duration returns old value, triggers refresh and returns new value
+ TimeUtil.setCurrentMillisSupplier(() -> 11);
+ assertThat(h2Cache.get("foo")).isEqualTo("load:foo");
+ assertThat(h2Cache.get("foo")).isEqualTo("reload:foo");
+ verify(baseLoader).reload("foo", "load:foo");
+ verifyNoMoreInteractions(baseLoader);
+ resetLoaderAndAnswerLoadAndRefreshCalls(baseLoader);
+
+ // Refreshed value was persisted
+ memCache.invalidateAll(); // Invalidates only the memcache, not the store.
+ assertThat(h2Cache.getIfPresent("foo")).isEqualTo("reload:foo");
+ }
+
+ private static void resetLoaderAndAnswerLoadAndRefreshCalls(CacheLoader<String, String> loader)
+ throws Exception {
+ reset(loader);
+ when(loader.load("foo")).thenReturn("load:foo");
+ when(loader.reload("foo", "load:foo")).thenReturn(Futures.immediateFuture("reload:foo"));
+ }
+
private static <K, V> Cache<K, ValueHolder<V>> disableMemCache() {
return CacheBuilder.newBuilder().maximumSize(0).build();
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
index b31ab9b..5ff7dde 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
@@ -90,10 +90,10 @@
line-height: var(--line-height-mono);
}
.u-green {
- color: var(--vote-text-color-recommended);
+ color: var(--positive-green-text-color);
}
.u-red {
- color: var(--vote-text-color-disliked);
+ color: var(--negative-red-text-color);
}
.u-gray-background {
background-color: var(--table-header-background-color);
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
index 1b18412..ca176be 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
@@ -74,10 +74,10 @@
color: #ffa62f;
}
.icon.invalid {
- color: var(--vote-text-color-disliked);
+ color: var(--negative-red-text-color);
}
.icon.trusted {
- color: var(--vote-text-color-recommended);
+ color: var(--positive-green-text-color);
}
.parentList.notCurrent.nonMerge #parentNotCurrentMessage {
--arrow-color: #ffa62f;
diff --git a/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.js b/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.js
index 0da31de..657915b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.js
+++ b/polygerrit-ui/app/elements/change/gr-change-requirements/gr-change-requirements_html.js
@@ -31,10 +31,10 @@
line-height: var(--line-height-mono);
}
.approved.status {
- color: var(--vote-text-color-recommended);
+ color: var(--positive-green-text-color);
}
.rejected.status {
- color: var(--vote-text-color-disliked);
+ color: var(--negative-red-text-color);
}
iron-icon {
color: inherit;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 05a3ae5..0b29298 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -2129,7 +2129,8 @@
_handleFileActionTap(e) {
e.preventDefault();
- const controls = this.$.fileListHeader.$.editControls;
+ const controls = this.$.fileListHeader
+ .shadowRoot.querySelector('#editControls');
const path = e.detail.path;
switch (e.detail.action) {
case GrEditConstants.Actions.DELETE.id:
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
index 6667ddb..d0b91e2 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
@@ -2000,7 +2000,10 @@
};
const fileList = element.$.fileList;
const Actions = GrEditConstants.Actions;
- const controls = element.$.fileListHeader.$.editControls;
+ element.$.fileListHeader.editMode = true;
+ flushAsynchronousOperations();
+ const controls = element.$.fileListHeader
+ .shadowRoot.querySelector('#editControls');
sandbox.stub(controls, 'openDeleteDialog');
sandbox.stub(controls, 'openRenameDialog');
sandbox.stub(controls, 'openRestoreDialog');
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.js
index 10b8606..bb04114 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.js
@@ -188,14 +188,16 @@
</div>
</div>
<div class$="rightControls [[_computeExpandedClass(filesExpanded)]]">
- <span class="showOnEdit flexContainer">
- <gr-edit-controls
- id="editControls"
- patch-num="[[patchNum]]"
- change="[[change]]"
- ></gr-edit-controls>
- <span class="separator"></span>
- </span>
+ <template is="dom-if" if="[[editMode]]">
+ <span class="showOnEdit flexContainer">
+ <gr-edit-controls
+ id="editControls"
+ patch-num="[[patchNum]]"
+ change="[[change]]"
+ ></gr-edit-controls>
+ <span class="separator"></span>
+ </span>
+ </template>
<span class$="[[_computeUploadHelpContainerClass(change, account)]]">
<gr-button link="" class="upload" on-click="_handleUploadTap"
>Update Change</gr-button
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
index 19362d5..2a5a302 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
@@ -299,13 +299,22 @@
});
test('edit-controls visibility', () => {
+ element.editMode = false;
+ flushAsynchronousOperations();
+ // on the first render, when editMode is false, editControls are not
+ // in the DOM to reduce size of DOM and make first render faster.
+ assert.isNull(element.shadowRoot
+ .querySelector('#editControls'));
+
element.editMode = true;
flushAsynchronousOperations();
- assert.isTrue(isVisible(element.$.editControls.parentElement));
+ assert.isTrue(isVisible(element.shadowRoot
+ .querySelector('#editControls').parentElement));
element.editMode = false;
flushAsynchronousOperations();
- assert.isFalse(isVisible(element.$.editControls.parentElement));
+ assert.isFalse(isVisible(element.shadowRoot
+ .querySelector('#editControls').parentElement));
});
test('_computeUploadHelpContainerClass', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
index cc4e934..ccf7f1a 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
@@ -151,10 +151,10 @@
min-width: 3.5em;
}
.added {
- color: var(--vote-text-color-recommended);
+ color: var(--positive-green-text-color);
}
.removed {
- color: var(--vote-text-color-disliked);
+ color: var(--negative-red-text-color);
text-align: left;
min-width: 4em;
padding-left: var(--spacing-s);
@@ -425,14 +425,14 @@
x$="[[_computeBarAdditionX(file, _sizeBarLayout)]]"
y="0"
height="8"
- fill="#388E3C"
+ fill="var(--positive-green-text-color)"
width$="[[_computeBarAdditionWidth(file, _sizeBarLayout)]]"
></rect>
<rect
x$="[[_computeBarDeletionX(_sizeBarLayout)]]"
y="0"
height="8"
- fill="#D32F2F"
+ fill="var(--negative-red-text-color)"
width$="[[_computeBarDeletionWidth(file, _sizeBarLayout)]]"
></rect>
</svg>
diff --git a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog.js b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog.js
index 132cd16..c42c734 100644
--- a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog.js
@@ -74,7 +74,9 @@
}
_computeGroups(includedIn, filterText) {
- if (!includedIn) { return []; }
+ if (!includedIn || filterText === undefined) {
+ return [];
+ }
const filter = item => !filterText.length ||
item.toLowerCase().indexOf(filterText.toLowerCase()) !== -1;
@@ -105,12 +107,6 @@
_computeLoadingClass(loaded) {
return loaded ? 'loading loaded' : 'loading';
}
-
- _onFilterChanged() {
- this.debounce('filter-change', () => {
- this._filterText = this.$.filterInput.bindValue;
- }, 100);
- }
}
customElements.define(GrIncludedInDialog.is, GrIncludedInDialog);
diff --git a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_html.js b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_html.js
index f5948e8..2faac52 100644
--- a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_html.js
+++ b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_html.js
@@ -73,12 +73,15 @@
>Close</gr-button
>
</span>
- <iron-input placeholder="Filter" on-bind-value-changed="_onFilterChanged">
+ <iron-input
+ id="filterInput"
+ placeholder="Filter"
+ bind-value="{{_filterText}}"
+ >
<input
- id="filterInput"
is="iron-input"
placeholder="Filter"
- on-bind-value-changed="_onFilterChanged"
+ bind-value="{{_filterText}}"
/>
</iron-input>
</header>
diff --git a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_test.html b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_test.html
index fe6119e..5d5b1fc 100644
--- a/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-included-in-dialog/gr-included-in-dialog_test.html
@@ -81,5 +81,20 @@
{title: 'Tags', items: ['v2.0', 'v2.1']},
]);
});
+
+ test('_computeGroups with .bindValue', done => {
+ element.$.filterInput.bindValue = 'stable-3.2';
+ const includedIn = {branches: [], tags: []};
+ includedIn.branches.push('master', 'stable-3.2');
+
+ setTimeout(() => {
+ const filterText = element._filterText;
+ assert.deepEqual(element._computeGroups(includedIn, filterText), [
+ {title: 'Branches', items: ['stable-3.2']},
+ ]);
+
+ done();
+ });
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.js b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.js
index 77148ad..e511101 100644
--- a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.js
+++ b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.js
@@ -53,7 +53,6 @@
--button-background-color,
var(--table-header-background-color)
);
- color: var(--primary-text-color);
padding: 0 var(--spacing-m);
@apply --vote-chip-styles;
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.js b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.js
index 21535d6..de4a72a 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.js
@@ -132,7 +132,7 @@
}
.score {
border-radius: var(--border-radius);
- color: var(--primary-text-color);
+ color: var(--vote-text-color);
display: inline-block;
padding: 0 var(--spacing-s);
text-align: center;
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_html.js b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_html.js
index 687dbd7..6aa7ffc 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_html.js
@@ -79,7 +79,7 @@
color: #1b5e20;
}
.submittableCheck {
- color: var(--vote-text-color-recommended);
+ color: var(--positive-green-text-color);
display: none;
}
.submittableCheck.submittable {
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.js b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.js
index 19e833c..2e64b3b 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.js
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.js
@@ -221,6 +221,8 @@
class="settingsButton"
href$="[[_generateSettingsLink()]]"
title="Settings"
+ aria-label="Settings"
+ role="button"
>
<iron-icon icon="gr-icons:settings"></iron-icon>
</a>
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_html.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_html.js
index 1d4b440..cc11cfa 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_html.js
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_html.js
@@ -51,7 +51,7 @@
}
}
</style>
- <span class="patchRange">
+ <span class="patchRange" aria-label="patch range starts with">
<gr-dropdown-list
id="basePatchDropdown"
value="[[basePatchNum]]"
@@ -67,8 +67,8 @@
>
</template>
</span>
- <span class="arrow">→</span>
- <span class="patchRange">
+ <span aria-hidden="true" class="arrow">→</span>
+ <span class="patchRange" aria-label="patch range ends with">
<gr-dropdown-list
id="patchNumDropdown"
value="[[patchNum]]"
diff --git a/polygerrit-ui/app/styles/gr-voting-styles.js b/polygerrit-ui/app/styles/gr-voting-styles.js
index 4860428..60bf623 100644
--- a/polygerrit-ui/app/styles/gr-voting-styles.js
+++ b/polygerrit-ui/app/styles/gr-voting-styles.js
@@ -26,6 +26,7 @@
box-shadow: none;
box-sizing: border-box;
min-width: 3em;
+ color: var(--vote-text-color);
}
}
</style>
diff --git a/polygerrit-ui/app/styles/themes/app-theme.js b/polygerrit-ui/app/styles/themes/app-theme.js
index 5d5d9e3..d76efe1 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.js
+++ b/polygerrit-ui/app/styles/themes/app-theme.js
@@ -38,10 +38,11 @@
--primary-button-text-color: white;
/* Used on text color for change list that doesn't need user's attention. */
--reviewed-text-color: black;
+ --vote-text-color: black;
--tooltip-text-color: white;
- --vote-text-color-recommended: #388e3c;
- --vote-text-color-disliked: #d32f2f;
-
+ --negative-red-text-color: #d93025;
+ --positive-green-text-color: #188038;
+
/* background colors */
/* primary background colors */
--background-color-primary: #ffffff;
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.html b/polygerrit-ui/app/styles/themes/dark-theme.html
index 4248878..f4cacd2 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.html
+++ b/polygerrit-ui/app/styles/themes/dark-theme.html
@@ -33,12 +33,13 @@
--deemphasized-text-color: #9aa0a6;
--default-button-text-color: #8ab4f8;
--error-text-color: red;
- --primary-button-text-color: var(--primary-text-color);
+ --primary-button-text-color: black;
/* Used on text color for change list doesn't need user's attention. */
--reviewed-text-color: #dadce0;
+ --vote-text-color: black;
--tooltip-text-color: white;
- --vote-text-color-recommended: #388e3c;
- --vote-text-color-disliked: #d32f2f;
+ --negative-red-text-color: #f28b82;
+ --positive-green-text-color: #81c995;
/* background colors */
/* primary background colors */
