You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/01/12 14:56:09 UTC
[GitHub] [druid] adrian-wang opened a new pull request #9166: use Caffeine
instead of Guava Cache
adrian-wang opened a new pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166
Fixes #8399 .
### Description
Turn to Caffeine instead of Guava's Cache in `ResourcePool`, `OnHeapLoadingCache`, and `SegmentLoadDropHandler`, and interface changes in `OffHeapLoadingCache`.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] drcrallen commented on issue #9166: use Caffeine instead of
Guava Cache
Posted by GitBox <gi...@apache.org>.
drcrallen commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-573785349
Thanks for the PR! One note: catching blanket `Exception` often breaks `InterruptedException` intentions, especially if the catch just continues on through the loop. Can you please make sure you are more prescriptive on the exceptions caught?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on a change in pull request #9166: use
Caffeine instead of Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on a change in pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#discussion_r367286494
##########
File path: extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/loading/LoadingCache.java
##########
@@ -21,19 +21,18 @@
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
-import com.google.common.cache.CacheLoader;
+import com.github.benmanes.caffeine.cache.CacheLoader;
import com.google.common.util.concurrent.ExecutionError;
import com.google.common.util.concurrent.UncheckedExecutionException;
import javax.annotation.Nullable;
import java.io.Closeable;
import java.util.Map;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
+import java.util.function.Function;
/**
* A semi-persistent mapping from keys to values. Cache entries are added using
- * {@link #get(Object, Callable)} and stored in the cache until either evicted or manually invalidated.
+ * {@link #get(Object, Function)} and stored in the cache until either evicted or manually invalidated.
* <p>
* <p>Implementations of this interface are expected to be thread-safe, and can be safely accessed
* by multiple concurrent threads.
Review comment:
Please ensure a warning is printed when legacy "guava" type is specified, and add a new "onHeap" type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on a change in pull request #9166: use
Caffeine instead of Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on a change in pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#discussion_r367286579
##########
File path: core/src/test/java/org/apache/druid/java/util/common/parsers/TimestampParserTest.java
##########
@@ -140,6 +140,7 @@ public void testTimeStampParserWithLongTimeZone()
long millis1 = new DateTime(1994, 11, 9, 4, 0, DateTimeZone.forOffsetHours(-8)).getMillis();
long millis2 = new DateTime(1994, 11, 9, 4, 0, DateTimeZone.forOffsetHours(-6)).getMillis();
+ // java.util.Locale.setDefault(java.util.Locale.US);
Review comment:
Please remove
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #9166: use Caffeine instead
of Guava Cache
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-578107415
Looks like there is a legit [checkstyle CI failure](https://travis-ci.org/apache/druid/jobs/639561475#L624):
```
[ERROR] /home/travis/build/apache/druid/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java:33:8: Unused import - java.util.concurrent.Callable. [UnusedImports]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adrian-wang commented on issue #9166: use Caffeine instead
of Guava Cache
Posted by GitBox <gi...@apache.org>.
adrian-wang commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-583222704
@leventov I've updated the code, now the failed checks are not relevant. can you please re-trigger the CI?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on issue #9166: use Caffeine instead of
Guava Cache
Posted by GitBox <gi...@apache.org>.
ccaominh commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-583236882
@adrian-wang I retriggered the jobs for you
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on a change in pull request #9166: use
Caffeine instead of Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on a change in pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#discussion_r367286627
##########
File path: core/src/test/java/org/apache/druid/java/util/common/parsers/TimestampParserTest.java
##########
@@ -157,6 +158,7 @@ public void testTimeStampParserWithLongTimeZone()
@Test
public void testTimeZoneAtExtremeLocations()
{
+ // java.util.Locale.setDefault(java.util.Locale.US);
Review comment:
And this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adrian-wang commented on issue #9166: use Caffeine instead
of Guava Cache
Posted by GitBox <gi...@apache.org>.
adrian-wang commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-609214996
@jihoonson Sorry for the late response, I just merged this with master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on issue #9166: use Caffeine instead of
Guava Cache
Posted by GitBox <gi...@apache.org>.
jihoonson commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-602246495
@adrian-wang sorry for the delayed review. Would you please fix the conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on issue #9166: use Caffeine instead of
Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-575042075
Labelling `Design Review` because the new name for cache property should be vetted (I suggest "onHeap" not to tie to a specific cache implementation, whether it is Guava or Caffeine).
Labelling `Release Notes` because it entails changes that cluster operators should apply: replace "guava" with "onHeap" (or whatever config name is chosen) and removing the concurrencyLevel property.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] adrian-wang commented on issue #9166: use Caffeine instead
of Guava Cache
Posted by GitBox <gi...@apache.org>.
adrian-wang commented on issue #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#issuecomment-576350983
@leventov @drcrallen Thanks for the review, I'm new to druid, for now I just resolved the conflicts. I'll take time to address your comments, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on a change in pull request #9166: use
Caffeine instead of Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on a change in pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#discussion_r367285150
##########
File path: extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/loading/OnHeapLoadingCache.java
##########
@@ -22,29 +22,24 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
import org.apache.druid.java.util.common.logger.Logger;
import java.util.Map;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
public class OnHeapLoadingCache<K, V> implements LoadingCache<K, V>
{
private static final Logger log = new Logger(OnHeapLoadingCache.class);
private static final int DEFAULT_INITIAL_CAPACITY = 16;
- //See com.google.common.cache.CacheBuilder#DEFAULT_CONCURRENCY_LEVEL
- private static final int DEFAULT_CONCURRENCY_LEVEL = 4;
private final Cache<K, V> cache;
private final AtomicBoolean isClosed = new AtomicBoolean(false);
@JsonProperty
- private final int concurrencyLevel;
Review comment:
Please make sure a warning is printed if this legacy JSON property is still specified.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] leventov commented on a change in pull request #9166: use
Caffeine instead of Guava Cache
Posted by GitBox <gi...@apache.org>.
leventov commented on a change in pull request #9166: use Caffeine instead of Guava Cache
URL: https://github.com/apache/druid/pull/9166#discussion_r367285602
##########
File path: extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
##########
@@ -96,28 +95,6 @@ public void testClose()
EasyMock.verify(lookupCache, reverseLookupCache);
}
- @Test
- public void testApplyWithExecutionError() throws ExecutionException
Review comment:
Could these tests be retained?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org