Refactor plugin code base
- The rateLimit class stores timeLapse as an integer.
PeriodicRateLimiter stores timeLapse as Long, which confuses
understanding and using the code. This change converts all timeLapse
variables to an integer.
- Remove timeLapse from warning rate limiters, since timeLapse can be
accesed from delegate
- Add methods getWarnLimit and getTimelapse
Change-Id: I0bb96c3c2309ee911a667d138992613d3cf36d67
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Configuration.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Configuration.java
index 1e1f7d7..db9c2e5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Configuration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Configuration.java
@@ -143,7 +143,7 @@
* @param rateLimitType type of rate limit
* @return map of rate limits per group uuid
*/
- Map<AccountGroup.UUID, RateLimit> getRatelimits(RateLimitType rateLimitType) {
+ Map<AccountGroup.UUID, RateLimit> getRateLimits(RateLimitType rateLimitType) {
return rateLimits != null ? rateLimits.row(rateLimitType) : ImmutableMap.of();
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Module.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Module.java
index 5597723..536a3d7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/Module.java
@@ -107,9 +107,9 @@
rateLimitType = limit.get().getType().getLimitType();
}
- long effectiveTimeLapse = PeriodicRateLimiter.DEFAULT_TIME_LAPSE_IN_MINUTES;
+ int effectiveTimeLapse = PeriodicRateLimiter.DEFAULT_TIME_LAPSE_IN_MINUTES;
if (timeLapse.isPresent()) {
- long providedTimeLapse = timeLapse.get().getRatePerHour();
+ int providedTimeLapse = timeLapse.get().getRatePerHour();
if (providedTimeLapse > 0 && providedTimeLapse <= effectiveTimeLapse) {
effectiveTimeLapse = providedTimeLapse;
rateLimitType = timeLapse.get().getType().getLimitType();
@@ -124,11 +124,10 @@
if (warn.isPresent()) {
if (limit.isPresent()) {
- return warningRateLimiterFactory.create(
- rateLimiter, key, warn.get().getRatePerHour(), effectiveTimeLapse);
+ return warningRateLimiterFactory.create(rateLimiter, key, warn.get().getRatePerHour());
}
return warningUnlimitedRateLimiterFactory.create(
- rateLimiter, key, warn.get().getRatePerHour(), effectiveTimeLapse);
+ rateLimiter, key, warn.get().getRatePerHour());
}
return rateLimiter;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiter.java
index 5c5ced3..0e99d83 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiter.java
@@ -16,6 +16,7 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Optional;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.Semaphore;
@@ -23,28 +24,33 @@
import java.util.concurrent.atomic.AtomicInteger;
class PeriodicRateLimiter implements RateLimiter {
- static final long DEFAULT_TIME_LAPSE_IN_MINUTES = 60L;
+ static final int DEFAULT_TIME_LAPSE_IN_MINUTES = 60;
private final Semaphore semaphore;
private final int maxPermits;
private final AtomicInteger usedPermits;
private final ScheduledFuture<?> replenishTask;
private final String rateLimitType;
+ private final int timeLapse;
interface Factory {
- PeriodicRateLimiter create(int permits, long timeLapse, String rateLimitType);
+ PeriodicRateLimiter create(
+ @Assisted("permits") Integer permits,
+ @Assisted("timeLapse") Integer timeLapse,
+ String rateLimitType);
}
@Inject
PeriodicRateLimiter(
@RateLimitExecutor ScheduledExecutorService executor,
- @Assisted int permits,
- @Assisted long timeLapse,
+ @Assisted("permits") Integer permits,
+ @Assisted("timeLapse") Integer timeLapse,
@Assisted String rateLimitType) {
this.semaphore = new Semaphore(permits);
this.maxPermits = permits;
this.usedPermits = new AtomicInteger();
this.rateLimitType = rateLimitType;
+ this.timeLapse = timeLapse;
this.replenishTask =
executor.scheduleAtFixedRate(
this::replenishPermits, timeLapse, timeLapse, TimeUnit.MINUTES);
@@ -91,6 +97,16 @@
}
@Override
+ public Optional<Integer> getTimeLapse() {
+ return Optional.of(timeLapse);
+ }
+
+ @Override
+ public Optional<Integer> getWarnLimit() {
+ return Optional.empty();
+ }
+
+ @Override
public void close() {
replenishTask.cancel(true);
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimitFinder.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimitFinder.java
index 4375f1a..0a9d8a0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimitFinder.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimitFinder.java
@@ -57,7 +57,7 @@
*/
private Optional<RateLimit> firstMatching(RateLimitType rateLimitType, IdentifiedUser user) {
Map<AccountGroup.UUID, RateLimit> limitsPerGroupUUID =
- configuration.getRatelimits(rateLimitType);
+ configuration.getRateLimits(rateLimitType);
if (!limitsPerGroupUUID.isEmpty()) {
GroupMembership memberShip = user.getEffectiveGroups();
for (Entry<AccountGroup.UUID, RateLimit> limitPerGroupUUID : limitsPerGroupUUID.entrySet()) {
@@ -76,7 +76,7 @@
*/
private Optional<RateLimit> getRateLimit(
RateLimitType rateLimitType, AccountGroup.UUID groupUUID) {
- Map<AccountGroup.UUID, RateLimit> limits = configuration.getRatelimits(rateLimitType);
+ Map<AccountGroup.UUID, RateLimit> limits = configuration.getRateLimits(rateLimitType);
return limits.isEmpty() ? Optional.empty() : Optional.ofNullable(limits.get(groupUUID));
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimiter.java
index d5455b1..b9943bf 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimiter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/RateLimiter.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.plugins.ratelimiter;
import java.util.Comparator;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
interface RateLimiter extends Comparable<RateLimiter> {
@@ -49,6 +50,12 @@
/** Return type of rate limiter * */
String getType();
+ /** Return time lapse of rate limiter. */
+ Optional<Integer> getTimeLapse();
+
+ /** Return warning limit of rate limiter. */
+ Optional<Integer> getWarnLimit();
+
/** Closes this RateLimiter, relinquishing any underlying resources. */
void close();
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/UnlimitedRateLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/UnlimitedRateLimiter.java
index d1989f4..8d4f60e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/UnlimitedRateLimiter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/UnlimitedRateLimiter.java
@@ -16,6 +16,7 @@
import static com.googlesource.gerrit.plugins.ratelimiter.Module.DEFAULT_RATE_LIMIT_TYPE;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
class UnlimitedRateLimiter implements RateLimiter {
@@ -55,6 +56,16 @@
}
@Override
+ public Optional<Integer> getTimeLapse() {
+ return Optional.empty();
+ }
+
+ @Override
+ public Optional<Integer> getWarnLimit() {
+ return Optional.empty();
+ }
+
+ @Override
public int usedPermits() {
return 0;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiter.java
index 68cc5ee..bdd8307 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiter.java
@@ -20,13 +20,14 @@
import com.google.inject.assistedinject.Assisted;
import java.time.LocalTime;
import java.time.format.DateTimeFormatter;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
import org.slf4j.Logger;
class WarningRateLimiter implements RateLimiter {
@FunctionalInterface
interface Factory {
- WarningRateLimiter create(RateLimiter delegate, String key, int warnLimit, long timeLapse);
+ WarningRateLimiter create(RateLimiter delegate, String key, int warnLimit);
}
private static final Logger rateLimitLog = RateLimiterStatsLog.getLogger();
@@ -37,7 +38,6 @@
private final RateLimitReachedSender.Factory rateLimitReachedSenderFactory;
private final int warnLimit;
private final String key;
- private final long timeLapse;
private volatile boolean wasLogged;
private volatile boolean warningWasLogged = false;
@@ -48,14 +48,12 @@
RateLimitReachedSender.Factory rateLimitReachedSenderFactory,
@Assisted RateLimiter delegate,
@Assisted String key,
- @Assisted int warnLimit,
- @Assisted long timeLapse) {
+ @Assisted int warnLimit) {
this.userResolver = userResolver;
this.delegate = delegate;
this.rateLimitReachedSenderFactory = rateLimitReachedSenderFactory;
this.warnLimit = warnLimit;
this.key = key;
- this.timeLapse = timeLapse;
}
@Override
@@ -70,7 +68,7 @@
String emailMessage =
String.format(
"User %s reached the warning limit of %s %s per %s minutes.",
- userResolver.getUserName(key).orElse(key), warnLimit, delegate.getType(), timeLapse);
+ userResolver.getUserName(key).orElse(key), warnLimit, delegate.getType(), delegate.getTimeLapse());
rateLimitLog.info(emailMessage);
warningWasLogged = true;
sendEmail(key, emailMessage, acquirePermit);
@@ -83,7 +81,7 @@
userResolver.getUserName(key).orElse(key),
permitsPerHour(),
delegate.getType(),
- timeLapse,
+ delegate.getTimeLapse(),
secondsToMsSs(remainingTime(TimeUnit.SECONDS)));
rateLimitLog.info(emailMessage);
wasLogged = true;
@@ -124,6 +122,11 @@
}
@Override
+ public Optional<Integer> getTimeLapse() {
+ return delegate.getTimeLapse();
+ }
+
+ @Override
public String getType() {
return delegate.getType();
}
@@ -142,6 +145,11 @@
return LocalTime.MIN.plusSeconds(seconds).format(format);
}
+ @Override
+ public Optional<Integer> getWarnLimit() {
+ return Optional.of(warnLimit);
+ }
+
@VisibleForTesting
public boolean getWarningFlagState() {
return warningWasLogged;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiter.java
index 7ce5be1..abf8966 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiter.java
@@ -17,14 +17,14 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
import org.slf4j.Logger;
class WarningUnlimitedRateLimiter implements RateLimiter {
@FunctionalInterface
interface Factory {
- WarningUnlimitedRateLimiter create(
- RateLimiter delegate, String key, int warnLimit, long timeLapse);
+ WarningUnlimitedRateLimiter create(RateLimiter delegate, String key, int warnLimit);
}
private static final Logger rateLimitLog = RateLimiterStatsLog.getLogger();
@@ -33,7 +33,6 @@
private final RateLimiter delegate;
private final int warnLimit;
private final String key;
- private final long timeLapse;
private volatile boolean warningWasLogged = false;
@Inject
@@ -41,13 +40,11 @@
UserResolver userResolver,
@Assisted RateLimiter delegate,
@Assisted String key,
- @Assisted int warnLimit,
- @Assisted long timeLapse) {
+ @Assisted int warnLimit) {
this.userResolver = userResolver;
this.delegate = delegate;
this.warnLimit = warnLimit;
this.key = key;
- this.timeLapse = timeLapse;
}
@Override
@@ -65,7 +62,7 @@
userResolver.getUserName(key).orElse(key),
warnLimit,
delegate.getType(),
- timeLapse);
+ delegate.getTimeLapse());
warningWasLogged = true;
}
return acquirePermit;
@@ -93,6 +90,11 @@
}
@Override
+ public Optional<Integer> getTimeLapse() {
+ return delegate.getTimeLapse();
+ }
+
+ @Override
public int usedPermits() {
return delegate.usedPermits();
}
@@ -102,6 +104,11 @@
delegate.close();
}
+ @Override
+ public Optional<Integer> getWarnLimit() {
+ return Optional.of(warnLimit);
+ }
+
@VisibleForTesting
public boolean getWarningFlagState() {
return warningWasLogged;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/ConfigurationTest.java b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/ConfigurationTest.java
index a4046b3..9c08d11 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/ConfigurationTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/ConfigurationTest.java
@@ -65,7 +65,7 @@
@Test
public void testEmptyConfig() {
- assertThat(getConfiguration().getRatelimits(RateLimitType.UPLOAD_PACK_PER_HOUR)).isEmpty();
+ assertThat(getConfiguration().getRateLimits(RateLimitType.UPLOAD_PACK_PER_HOUR)).isEmpty();
}
@Test
@@ -77,7 +77,7 @@
validRate);
Map<AccountGroup.UUID, RateLimit> rateLimit =
- getConfiguration().getRatelimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
+ getConfiguration().getRateLimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
assertThat(rateLimit).hasSize(1);
assertThat(rateLimit.get(someGroupDescMock.getGroupUUID()).getRatePerHour())
.isEqualTo(validRate);
@@ -129,7 +129,7 @@
"badGroup");
Map<AccountGroup.UUID, RateLimit> rateLimit =
- getConfiguration().getRatelimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
+ getConfiguration().getRateLimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
assertThat(rateLimit).hasSize(1);
assertThat(rateLimit.get(someGroupDescMock.getGroupUUID()).getRatePerHour())
.isEqualTo(validRate);
@@ -140,7 +140,7 @@
globalPluginConfig.fromText("[group \"Administrators\"]");
Map<AccountGroup.UUID, RateLimit> rateLimit =
- getConfiguration().getRatelimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
+ getConfiguration().getRateLimits(RateLimitType.UPLOAD_PACK_PER_HOUR);
assertThat(rateLimit).hasSize(1);
assertThat(rateLimit.get(administratorsGroupDescMock.getGroupUUID())).isNull();
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiterTest.java b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiterTest.java
index b949490..15ad0d8 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiterTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/PeriodicRateLimiterTest.java
@@ -71,8 +71,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
any(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
}
@@ -82,8 +82,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
runnableCaptor.capture(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
// Use all permits
diff --git a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiterTest.java b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiterTest.java
index dc19df0..f03e745 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiterTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningRateLimiterTest.java
@@ -63,8 +63,8 @@
rateLimitReachedSenderFactory,
limiter,
"dummy",
- WARN_RATE,
- DEFAULT_TIME_LAPSE_IN_MINUTES);
+ WARN_RATE
+ );
}
@Test
@@ -73,6 +73,11 @@
}
@Test
+ public void testGetWarningLimit() {
+ assertThat(warningLimiter.getWarnLimit().get()).isEqualTo(WARN_RATE);
+ }
+
+ @Test
public void testAcquireAll() {
assertThat(warningLimiter.availablePermits()).isEqualTo(RATE);
@@ -111,8 +116,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
any(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
}
@@ -122,8 +127,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
runnableCaptor.capture(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
replenishPermits(warningLimiter, runnableCaptor);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiterTest.java b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiterTest.java
index 7717234..7ed1d93 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiterTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/ratelimiter/WarningUnlimitedRateLimiterTest.java
@@ -42,8 +42,7 @@
new PeriodicRateLimiter(
scheduledExecutorMock, RATE, DEFAULT_TIME_LAPSE_IN_MINUTES, "Any Type");
warningUnlimitedLimiter =
- new WarningUnlimitedRateLimiter(
- userResolver, limiter, "dummy", WARN_RATE, DEFAULT_TIME_LAPSE_IN_MINUTES);
+ new WarningUnlimitedRateLimiter(userResolver, limiter, "dummy", WARN_RATE);
}
@Test
@@ -75,8 +74,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
any(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
}
@@ -86,8 +85,8 @@
verify(scheduledExecutorMock)
.scheduleAtFixedRate(
runnableCaptor.capture(),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
- eq(DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
+ eq((long) DEFAULT_TIME_LAPSE_IN_MINUTES),
eq(TimeUnit.MINUTES));
testTriggerWarning();
