You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2023/02/25 18:13:57 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3206: Replace all instances of Guava cache with Caffeine
cshannon opened a new pull request, #3206:
URL: https://github.com/apache/accumulo/pull/3206
[Caffeine](https://github.com/ben-manes/caffeine) is the replacement for the Guava cache api and has several improvements including efficiency with its admission [policy](https://github.com/ben-manes/caffeine/wiki/Efficiency) and [performance](https://github.com/ben-manes/caffeine/wiki/Benchmarks). Currently the Accumulo code base has a mixture where some places use Guava and some places use Caffeine so it is not consistent. Switching to Caffeine is nearly a drop in replacement and this commit replaces all instances of Guava with Caffeine for consistency and also adds a checkstyle rule to prevent using Guava in the future.
From the Guava [docs](https://guava.dev/releases/31.1-jre/api/docs/com/google/common/cache/CacheBuilder.html) regarding Caffeine:
> **Prefer [Caffeine](https://github.com/ben-manes/caffeine/wiki) over Guava's caching API**
**The successor to Guava's caching API is [Caffeine](https://github.com/ben-manes/caffeine/wiki). Its API is designed to make it a nearly drop-in replacement** -- though it requires Java 8 APIs, is not available for Android or GWT/j2cl, and may have [different (usually better) behavior](https://github.com/ben-manes/caffeine/wiki/Guava) when multiple threads attempt concurrent mutations. Its equivalent to CacheBuilder is its [Caffeine](https://www.javadoc.io/doc/com.github.ben-manes.caffeine/caffeine/latest/com.github.benmanes.caffeine/com/github/benmanes/caffeine/cache/Caffeine.html) class. **Caffeine offers better performance, more features (including asynchronous loading), and fewer [bugs](https://github.com/google/guava/issues?q=is%3Aopen+is%3Aissue+label%3Apackage%3Dcache+label%3Atype%3Ddefect).**
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon merged pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #3206:
URL: https://github.com/apache/accumulo/pull/3206
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#issuecomment-1446274242
Kicked off full IT build
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#discussion_r1124672058
##########
pom.xml:
##########
@@ -1129,6 +1129,10 @@
<property name="format" value="&quot; [+] &quot;" />
<property name="message" value="Unnecessary concatenation of string literals" />
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="com.google.common.cache[.]" />
Review Comment:
Good catch
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#discussion_r1122419787
##########
core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java:
##########
@@ -63,19 +62,15 @@ public ClassLoader getClassLoader(String context) {
throw new IllegalArgumentException("Unknown context");
}
- try {
- return classloaders.get(context, () -> {
- LOG.debug("Creating URLClassLoader for context, uris: {}", context);
- return new URLClassLoader(Arrays.stream(context.split(",")).map(url -> {
- try {
- return new URL(url);
- } catch (MalformedURLException e) {
- throw new RuntimeException(e);
- }
- }).collect(Collectors.toList()).toArray(new URL[] {}), ClassLoader.getSystemClassLoader());
- });
- } catch (ExecutionException e) {
- throw new RuntimeException(e);
- }
+ return classloaders.get(context, k -> {
+ LOG.debug("Creating URLClassLoader for context, uris: {}", context);
+ return new URLClassLoader(Arrays.stream(context.split(",")).map(url -> {
+ try {
+ return new URL(url);
+ } catch (MalformedURLException e) {
+ throw new RuntimeException(e);
+ }
+ }).collect(Collectors.toList()).toArray(new URL[] {}), ClassLoader.getSystemClassLoader());
Review Comment:
Good catch, I will go ahead and fix that.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#discussion_r1119099598
##########
core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java:
##########
@@ -63,19 +62,15 @@ public ClassLoader getClassLoader(String context) {
throw new IllegalArgumentException("Unknown context");
}
- try {
- return classloaders.get(context, () -> {
- LOG.debug("Creating URLClassLoader for context, uris: {}", context);
- return new URLClassLoader(Arrays.stream(context.split(",")).map(url -> {
- try {
- return new URL(url);
- } catch (MalformedURLException e) {
- throw new RuntimeException(e);
- }
- }).collect(Collectors.toList()).toArray(new URL[] {}), ClassLoader.getSystemClassLoader());
- });
- } catch (ExecutionException e) {
- throw new RuntimeException(e);
- }
+ return classloaders.get(context, k -> {
+ LOG.debug("Creating URLClassLoader for context, uris: {}", context);
+ return new URLClassLoader(Arrays.stream(context.split(",")).map(url -> {
+ try {
+ return new URL(url);
+ } catch (MalformedURLException e) {
+ throw new RuntimeException(e);
+ }
+ }).collect(Collectors.toList()).toArray(new URL[] {}), ClassLoader.getSystemClassLoader());
Review Comment:
```suggestion
}).toArray(URL[]::new), ClassLoader.getSystemClassLoader());
```
Outside of the scope and not changed in this PR but noticed that the collect step can be skipped here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#discussion_r1124620305
##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/CachableBlockFile.java:
##########
@@ -170,8 +169,14 @@ public static class Reader implements Closeable {
private long getCachedFileLen() throws IOException {
try {
- return fileLenCache.get(cacheId, lengthSupplier::get);
- } catch (ExecutionException e) {
+ return fileLenCache.get(cacheId, k -> {
+ try {
+ return lengthSupplier.get();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ } catch (Exception e) {
Review Comment:
@cshannon Can this catch block be narrowed to just the UncheckedIOException that you're throwing internally, or in the worst case, catching all RTEs? I don't see what checked exceptions this needs to catch.
##########
server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java:
##########
@@ -144,8 +144,14 @@ private void initiateSort(String sortId, String source, final String destination
private boolean exists(final Path path) throws IOException {
try {
- return existenceCache.get(path, () -> manager.getVolumeManager().exists(path));
- } catch (ExecutionException e) {
+ return existenceCache.get(path, k -> {
+ try {
+ return manager.getVolumeManager().exists(path);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ } catch (Exception e) {
Review Comment:
Again, a very broad catch.
##########
pom.xml:
##########
@@ -1129,6 +1129,10 @@
<property name="format" value="&quot; [+] &quot;" />
<property name="message" value="Unnecessary concatenation of string literals" />
</module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="com.google.common.cache[.]" />
Review Comment:
The pattern should be: `"com[.]google[.]common[.]cache[.]"
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#issuecomment-1446783775
Full IT build passed successfully
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3206: Replace all instances of Guava cache with Caffeine
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3206:
URL: https://github.com/apache/accumulo/pull/3206#discussion_r1124658802
##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/CachableBlockFile.java:
##########
@@ -170,8 +169,14 @@ public static class Reader implements Closeable {
private long getCachedFileLen() throws IOException {
try {
- return fileLenCache.get(cacheId, lengthSupplier::get);
- } catch (ExecutionException e) {
+ return fileLenCache.get(cacheId, k -> {
+ try {
+ return lengthSupplier.get();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ } catch (Exception e) {
Review Comment:
I'll take a look, I can create a follow up PR with requested changes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org