From 2de70533c9acbd79ac8efc3b1f79e82bcfde59ec Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 4 Nov 2021 13:42:18 -0400 Subject: [PATCH 1/2] Revert "Feature: Zero Warmup Guava RateLimiter (#1950)" This reverts commit 3219b6cacf65aa96895d25d3e5ed852b41a4726b. Warmup of zero results in no rate limiting. --- README.md | 1 - .../errorprone/NoWarmupRateLimiter.java | 72 ----------- .../errorprone/NoWarmupRateLimiterTest.java | 113 ------------------ .../BaselineErrorProneExtension.java | 1 - 4 files changed, 187 deletions(-) delete mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NoWarmupRateLimiter.java delete mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/NoWarmupRateLimiterTest.java diff --git a/README.md b/README.md index 5ccefaa95..3b7207248 100644 --- a/README.md +++ b/README.md @@ -206,7 +206,6 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `PreferImmutableStreamExCollections`: It's common to use toMap/toSet/toList() as the terminal operation on a stream, but would be extremely surprising to rely on the mutability of these collections. Prefer `toImmutableMap`, `toImmutableSet` and `toImmutableList`. (If the performance overhead of a stream is already acceptable, then the `UnmodifiableFoo` wrapper is likely tolerable). - `DangerousIdentityKey`: Key type does not override equals() and hashCode, so comparisons will be done on reference equality only. - `ConsistentOverrides`: Ensure values are bound to the correct variables when overriding methods -- `NoWarmupRateLimiter`: Ensures Guava RateLimiter initializers specify a warm-up duration. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NoWarmupRateLimiter.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NoWarmupRateLimiter.java deleted file mode 100644 index 947b4302f..000000000 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NoWarmupRateLimiter.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. - * - * 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.palantir.baseline.errorprone; - -import com.google.auto.service.AutoService; -import com.google.errorprone.BugPattern; -import com.google.errorprone.BugPattern.LinkType; -import com.google.errorprone.BugPattern.SeverityLevel; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.fixes.SuggestedFixes; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.matchers.method.MethodMatchers; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import java.util.List; - -@AutoService(BugChecker.class) -@BugPattern( - name = "NoWarmupRateLimiter", - link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", - linkType = LinkType.CUSTOM, - severity = SeverityLevel.WARNING, - summary = "Ensures Guava RateLimiter initializers specify a warm-up duration. The default Guava RateLimiter " - + "without specifying a warm-up time creates a limiter with 0 permits, and starts acquiring them. " - + "This causes an unexpected behavior, where at the beginning of the rate limiter's lifetime, the " - + "rate is much lower than specified. Zero warm-up time should be preferred to no warm-up time to " - + "ensure a max amount of permits are available at the start.") -public final class NoWarmupRateLimiter extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { - - private static final String DURATION_NAME = "java.time.Duration"; - private static final String RATE_LIMITER_NAME = "com.google.common.util.concurrent.RateLimiter"; - private static final Matcher RATELIMITER_CREATE_METHOD = MethodMatchers.staticMethod() - .onClass(RATE_LIMITER_NAME) - .named("create") - .withParameters("double"); - - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (RATELIMITER_CREATE_METHOD.matches(tree, state)) { - List args = tree.getArguments(); - if (args.size() > 1) { - return Description.NO_MATCH; - } - SuggestedFix.Builder fix = SuggestedFix.builder(); - String durationType = SuggestedFixes.qualifyType(state, fix, DURATION_NAME); - fix.replace(args.get(0), String.format("%s, %s.ZERO", state.getSourceForNode(args.get(0)), durationType)); - return buildDescription(tree) - .setMessage("RateLimiter.create() does not include a warmup time. " - + "Consider including Duration.ZERO as the default warmup.") - .addFix(fix.build()) - .build(); - } - return Description.NO_MATCH; - } -} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/NoWarmupRateLimiterTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/NoWarmupRateLimiterTest.java deleted file mode 100644 index f021055b9..000000000 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/NoWarmupRateLimiterTest.java +++ /dev/null @@ -1,113 +0,0 @@ -/* - * (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. - * - * 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.palantir.baseline.errorprone; - -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -public class NoWarmupRateLimiterTest { - - @Test - public void should_explicitly_initialize_warmup() { - getRefactoringValidatorHelper() - .addInputLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "class Test {", - " void f() {", - " RateLimiter.create(10);", - " }", - "", - "}") - .addOutputLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "import java.time.Duration;", - "class Test {", - " void f() {", - " RateLimiter.create(10, Duration.ZERO);", - " }", - "", - "}") - .doTest(); - } - - @Test - public void should_explicitly_initialize_warmup_import_exists() { - getRefactoringValidatorHelper() - .addInputLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "import java.time.Duration;", - "class Test {", - " void f() {", - " RateLimiter.create(10);", - " }", - "", - "}") - .addOutputLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "import java.time.Duration;", - "class Test {", - " void f() {", - " RateLimiter.create(10, Duration.ZERO);", - " }", - "", - "}") - .doTest(); - } - - @Test - public void pass_explicitly_initialize_warmup_single_arg() { - getCompilationTestHelper() - .addSourceLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "import java.time.Duration;", - "class Test {", - " void f() {", - " RateLimiter.create(10, Duration.ZERO);", - " }", - "", - "}") - .doTest(); - } - - @Test - public void pass_explicitly_initialize_warmup_double_args() { - getCompilationTestHelper() - .addSourceLines( - "Test.java", - "import com.google.common.util.concurrent.RateLimiter;", - "import java.util.concurrent.TimeUnit;", - "class Test {", - " void f() {", - " RateLimiter.create(10, 100, TimeUnit.SECONDS);", - " }", - "", - "}") - .doTest(); - } - - private RefactoringValidator getRefactoringValidatorHelper() { - return RefactoringValidator.of(NoWarmupRateLimiter.class, getClass()); - } - - private CompilationTestHelper getCompilationTestHelper() { - return CompilationTestHelper.newInstance(NoWarmupRateLimiter.class, getClass()); - } -} diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 9005e877d..48e49b8d5 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -47,7 +47,6 @@ public class BaselineErrorProneExtension { "LambdaMethodReference", "LoggerEnclosingClass", "LogsafeArgName", - "NoWarmupRateLimiter", "ObjectsHashCodeUnnecessaryVarargs", "OptionalFlatMapOfNullable", "OptionalOrElseMethodInvocation", From 2f1ab4be5f855f9d99171d24b0f01146af077052 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Thu, 4 Nov 2021 17:43:07 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-1957.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-1957.v2.yml diff --git a/changelog/@unreleased/pr-1957.v2.yml b/changelog/@unreleased/pr-1957.v2.yml new file mode 100644 index 000000000..4584c10e6 --- /dev/null +++ b/changelog/@unreleased/pr-1957.v2.yml @@ -0,0 +1,6 @@ +type: fix +fix: + description: 'Revert "Feature: Zero Warmup Guava RateLimiter (#1950)" which results + in no rate limiting' + links: + - https://github.com/palantir/gradle-baseline/pull/1957