You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "amarkevich (via GitHub)" <gi...@apache.org> on 2023/07/05 22:23:18 UTC
[GitHub] [activemq-artemis] amarkevich opened a new pull request, #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
amarkevich opened a new pull request, #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540
based on benchmark https://github.com/ben-manes/caffeine/wiki/Benchmarks
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624272168
Guava itself is much more than just the cache, but if you're comparing Guava Cache vs. Caffeine then I think more folks are using Caffeine.
Also, since Guava is a lot more than just a cache that means Caffeine is much smaller and will likely reduce risk of CVEs.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1632321610
Both (once you restore the console bit you have removed)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1645339199
FWIW I dont see #4545 as a pre-requisite to this PR. It may get rid of the build error, in a different way, but it doesnt change the fixes/restorations still needing made here, which would also get rid of the build problem even without the other PR.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1625726505
It may completes but as per earlier comments it isn't really 'successfull'; the compiled code then doesnt actually work because all the code wasnt generated, since the change stopped the logging annotation processor from running at all, meaning there are missing classes that stop various modules (client, broker, etc) from working.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1625639547
> still failing to build
without the latest commit build is successful
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624160862
well.. at least some initial googling shows that they are active on fixing this:
https://security.snyk.io/package/maven/com.github.ben-manes.caffeine:caffeine
any opinions?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255713719
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
Content of artemis-console-2.30.0-SNAPSHOT.war\WEB-INF\lib :
checker-qual-3.5.0.jar
commons-codec-1.15.jar
commons-io-2.11.0.jar
commons-logging-1.2.jar
error_prone_annotations-2.3.4.jar
failureaccess-1.0.1.jar
hawtio-core-2.15.0.jar
hawtio-system-2.15.0.jar
hawtio-util-2.15.0.jar
httpclient-4.5.13.jar
httpcore-4.4.13.jar
j2objc-annotations-1.3.jar
jolokia-core-1.7.0.jar
json-20171018.jar
json-simple-1.1.1.jar
jsr305-3.0.2.jar
listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1631595915
> want the guava dependencyManagement restored
for the moment - yes, keep `annotationProcessorPaths` for further change
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1626358483
> stopped the logging annotation processor
let me check this regression
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255868866
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
```
activemq-artemis\artemis-quorum-ri>mvn dependency:tree -Derrorprone
[INFO] --- dependency:3.6.0:tree (default-cli) @ artemis-quorum-ri ---
[INFO] org.apache.activemq:artemis-quorum-ri:jar:2.30.0-SNAPSHOT
[INFO] +- org.apache.curator:curator-recipes:jar:5.2.0:compile
[INFO] | \- org.apache.curator:curator-framework:jar:5.2.0:compile
[INFO] +- org.apache.curator:curator-client:jar:5.2.0:compile
[INFO] | \- com.google.guava:guava:jar:27.0.1-jre:compile
[INFO] | +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] | +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] | +- org.checkerframework:checker-qual:jar:2.5.2:compile
[INFO] | +- com.google.j2objc:j2objc-annotations:jar:1.1:compile
[INFO] | \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.17:compile
[INFO] +- org.apache.zookeeper:zookeeper:jar:3.6.3:compile
[INFO] | +- org.apache.zookeeper:zookeeper-jute:jar:3.6.3:compile
[INFO] | \- io.netty:netty-handler:jar:4.1.94.Final:compile
[INFO] | +- io.netty:netty-common:jar:4.1.94.Final:compile
[INFO] | +- io.netty:netty-resolver:jar:4.1.94.Final:compile
[INFO] | +- io.netty:netty-transport-native-unix-common:jar:4.1.94.Final:compile
[INFO] | \- io.netty:netty-codec:jar:4.1.94.Final:compile
[INFO] +- org.apache.curator:curator-test:jar:5.2.0:test
[INFO] | +- io.dropwizard.metrics:metrics-core:jar:3.2.5:test
[INFO] | \- org.xerial.snappy:snappy-java:jar:1.1.7:test
[INFO] +- org.apache.activemq:artemis-quorum-api:jar:2.30.0-SNAPSHOT:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.36:compile
[INFO] +- org.apache.logging.log4j:log4j-slf4j-impl:jar:2.20.0:test
[INFO] | +- org.apache.logging.log4j:log4j-api:jar:2.20.0:test
[INFO] | \- org.apache.logging.log4j:log4j-core:jar:2.20.0:test
[INFO] +- org.apache.activemq:artemis-commons:jar:2.30.0-SNAPSHOT:compile
[INFO] | +- io.netty:netty-buffer:jar:4.1.94.Final:compile
[INFO] | +- io.netty:netty-transport:jar:4.1.94.Final:compile
[INFO] | \- commons-beanutils:commons-beanutils:jar:1.9.4:compile
[INFO] | +- commons-logging:commons-logging:jar:1.2:compile
[INFO] | \- commons-collections:commons-collections:jar:3.2.2:compile
[INFO] +- junit:junit:jar:4.13.2:test
[INFO] | \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] +- org.apache.activemq:artemis-unit-test-support:jar:2.30.0-SNAPSHOT:test
[INFO] +- org.hamcrest:hamcrest:jar:2.1:test
[INFO] \- com.google.errorprone:error_prone_core:jar:2.20.0:provided
[INFO] +- com.google.errorprone:error_prone_annotation:jar:2.20.0:provided
[INFO] +- com.google.errorprone:error_prone_type_annotations:jar:2.20.0:provided
[INFO] +- com.google.errorprone:error_prone_check_api:jar:2.20.0:provided
[INFO] | +- io.github.java-diff-utils:java-diff-utils:jar:4.0:provided
[INFO] | | \- org.eclipse.jgit:org.eclipse.jgit:jar:4.4.1.201607150455-r:provided
[INFO] | +- com.github.kevinstern:software-and-algorithms:jar:1.0:provided
[INFO] | +- com.github.ben-manes.caffeine:caffeine:jar:3.1.6:provided
[INFO] | \- com.google.inject:guice:jar:5.1.0:provided
[INFO] | \- aopalliance:aopalliance:jar:1.0:provided
[INFO] +- org.pcollections:pcollections:jar:3.1.4:provided
[INFO] +- com.google.auto:auto-common:jar:1.2.1:provided
[INFO] +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] +- io.github.eisop:dataflow-errorprone:jar:3.34.0-eisop1:provided
[INFO] +- com.google.auto.value:auto-value-annotations:jar:1.9:provided
[INFO] +- com.google.errorprone:error_prone_annotations:jar:2.20.0:compile
[INFO] +- com.google.protobuf:protobuf-java:jar:3.19.6:provided
[INFO] +- com.google.auto.service:auto-service-annotations:jar:1.0.1:provided
[INFO] \- javax.inject:javax.inject:jar:1:provided
```
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256183637
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
>[INFO] +- org.apache.curator:curator-client:jar:5.2.0:compile
[INFO] | \- com.google.guava:guava:jar:27.0.1-jre:compile
So that module is now using Guava 27.0.1 (instead of 32.0.1 as before), which presumably doesn't have the thing needed by ErrorProne, which normally uses a newer version.
Maven is picking the older version during resolution, probably because curator-client is earlier on the dependency set than the ErrorProne dep is (it will be last due to being in a profile). Its doing that instead of just using the specified dependencyManagement guava version that was removed from the build. Restoring that may fix things. Alternative perhaps there is a newer curator-client version that uses a recent guava.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256604068
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
still I can't reproduce error using `annotationProcessorPaths` approach
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624181340
Caffeine is essentially the successor to Google's Guava Cache. It's used by a lot of projects. Spring, for example, started moving [away from Guava to Caffeine in 2015](https://github.com/spring-projects/spring-framework/issues/18370). I considered using Caffeine when I first started using Guava Cache on [ARTEMIS-2886](https://issues.apache.org/jira/browse/ARTEMIS-2886), but I decided on Guava mainly because we already had a dependency on it. This PR completely removes Guava from the code-base in lieu of Caffeine which is fine by me.
My only real concern is essentially the same as Tim's which I put on [ARTEMIS-4349](https://issues.apache.org/jira/browse/ARTEMIS-4349?focusedCommentId=17740337&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17740337). Changing Caffeine for the sake of performance doesn't make much sense without clear data that changing will actually make any meaningful difference. At this point we don't have that data.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256602437
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
same for now - no `checker-compat-qual` in war
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624271282
I'm running the whole test suite with these changes (on a separate CI).
If caffeine has a bigger community, the jar is smaller, I think we should bring this right away (if tests are ok of course).
my initial concern was about the community support and CVEs.. but from what I later gathered this is actually better.
performance wise I think the differences would be impossible to measure. We currently use some login changes from Security as Justin mentioned... but you would have to use the cache quite a lot to have any significant result.
I think we should merge this, but not much for the performance reasons.
anyone has anything against 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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255788536
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
When compiling what? That looks like part of Guava, which is an Error Prone dependency...so did something exclude it?
This change [silently] breaks the compilation [output] too, so...
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1629809232
> still not building
yep, going to do https://github.com/apache/activemq-artemis/pull/4545 first
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1259992802
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
I'd guess because it is doing a completely different thing there, with the ErrorProne deps defined earlier off-classpath, likely meaning its transitive guava dep version gets picked up, before starting to look at the compile deps on the classpath...rather than the reverse in the profile case. Which is fine if you dont remove the dependencyManagement entries ensuring the desired versions are chosen.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1632188903
> > want the guava dependencyManagement restored
>
> for the moment - yes, keep `annotationProcessorPaths` for further change
Regardless of the annotationProcessorPaths situation. Guava will still be getting used in the project after this change, no matter which way the ErrorProne etc annotation processors are being configured (or even if we removed ErrorProne entirely).
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1632206994
> Guava will still be getting used in the project
directly or as transitive dependency?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1687959386
I raised a new PR, #4584, to address the various trivial commented build etc issues on this PR which were preventing this progressing. Unfortunately it turns out this PR was still failing a test (MQTTSecurityManagerTest.testSecurityManagerModifyClientIDAndStealConnection) in its current state, and so the new PR is also and also cant be merged without more work first to identify+fix that issue.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255722522
##########
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java:
##########
@@ -380,7 +380,7 @@ private static String formatByte(byte bite) {
}
private static String formatCase(String string) {
- return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, string);
+ return CaseUtils.toCamelCase(string, false, ' ');
Review Comment:
fixed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255861464
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
https://github.com/apache/activemq-artemis/actions/runs/5487090252/jobs/9998059886?pr=4540
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1646681855
> pre-requisite to this PR
its not a blocker but allow to implement current changes easy way
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] tabish121 commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624175542
I'm no fan of guava but my first question on any change like this is what benefit if any does it actually impart? Is there some measurable performance improvement worth changing it now vs at some later stage like a major version bump where these types of changes are more expected? (e.g. Artemis 3.0)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ben-manes commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624526398
Hi,
If you do not have a performance reason to switch then staying with Guava's is perfectly fine. I would not use their `asMap().compute` as a wonky implementation given it was added so late (and you don't use it, so that's fine). Guava's is robust but not actively maintained (CacheBuilder's JavaDoc recommends Caffeine).
The most common cause for tests to fail during a migration is that Caffeine switches eviction and listeners to be executed async by default, whereas Guava piggybacks on the calling thread. You can emulate that using `Caffeine.executor(Runnable::run)` if desired. The problem is that often tests assumed no concurrency (immediate eviction or callbacks invoked) and that is no longer a valid assumption. In both cache's their internal logic is very inexpensive (e.g. LRU reordering), but we do not know the cost of the user's callback. Since the JVM now offers shared threads and Caffeine offers an AsyncCache, defaulting to async to minimize user-facing latencies seemed like a reasonable choice. There are other subtle differences, such as if assuming lru eviction order, so you might want to review this [migration page](https://github.com/ben-manes/caffeine/wiki/Guava).
Whatever you decide to do is fine with me. Feel welcome to reach out if you have any questions or concerns.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1631139595
Presumably #4545 helps this PR not error as-is, since it uses a newer version of guava than the 27.0.1 that the old 5.2.0 curator did, one new enough to satisfy error-prones needs? It is however still using an old version of guava, 31.1-jre, so note we will still want the guava dependencyManagement restored to bring it back up to the 32.0.1-jre release it was at before this PR.
In hindsight, removing the guava from the console war may not have broken the console in the assembly, since guava is still going to be brought in to the assembly via the quorum bits. It should still remain in the war though as the server itself doesnt bring it in with these changes, and the console can be/is used with the server without using the assembly.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256183637
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
So that module is now using Guava 27, which presumably doesn't have the thing needed by ErrorProne, which normally uses a newer version.
Maven is picking the older version during resolution, probably because it is earlier on the dependency set than the ErrorProne dep is (it will be last due to being in a profile). Its doing that instead of just using the specified dependencyManagement guava version that was removed from the build. Restoring that may fix things. Alternative perhaps there is a newer curator-client version that uses a recent guava.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624235147
@jbertram based on what you said it sounds like caffeine has a bigger community than guava? that would be a good reason for the replacement by itself.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255727606
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
ErrorProne configuration changed because produce `java.lang.NoClassDefFoundError: com/google/common/collect/ImmutableMap` during compilation
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255826410
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
`checker-compat-qual` is not there
BTW artemis-console-2.29.0.war\WEB-INF\lib\ contains
checker-qual-3.12.0.jar
checker-qual-3.5.0.jar
Should be `checker-qual` excluded then?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1625607369
All the arguments seems to indicate we should do the replacement.
However this is still failing to build. I have a pipeline where I execute the whole test suite:
"[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project artemis-quorum-ri: Compilation failure
[ERROR] An unknown compilation problem occurred
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR]
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn <args> -rf :artemis-quorum-ri
[Pipeline] }"
This is also translated as errors on the build here.
to test this locally you could do:
```
mvn clean; mvn install -DskipTests=true -Pdev -Derrorprone
```
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256175426
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
The checker-compat-qual is presumably not there because it was excluded, per the comment in your removal. Having 2 checker-qual versions would seem like an oversight though, not noticed during an update.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1629777354
still not building:
[INFO] Compiling 6 source files with javac [debug target 11] to target/classes
compiler message file broken: key=compiler.misc.msg.bug arguments=11.0.8, {1}, {2}, {3}, {4}, {5}, {6}, {7}
java.lang.NoSuchMethodError: 'com.google.common.collect.ImmutableMap com.google.common.collect.ImmutableMap$Builder.buildOrThrow()'
at com.google.errorprone.scanner.ScannerSupplier.defaultSeverities(ScannerSupplier.java:67)
at com.google.errorprone.scanner.ScannerSupplier.fromBugCheckerInfos(ScannerSupplier.java:88)
at com.google.errorprone.scanner.BuiltInCheckerSuppliers.allChecks(BuiltInCheckerSuppliers.java:600)
at com.google.errorprone.scanner.BuiltInCheckerSuppliers.defaultChecks(BuiltInCheckerSuppliers.java:608)
at com.google.errorprone.ErrorProneJavacPlugin.init(ErrorProneJavacPlugin.java:35)
at jdk.compiler/com.sun.tools.javac.api.BasicJavacTask.initPlugins(BasicJavacTask.java:215)
at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.prepareCompiler(JavacTaskImpl.java:199)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1647499364
>
> its not a blocker but allow to implement current changes easy way
From what I can see the acceptable changes on this PR will be the same, independent of what happens on the other PR. They could have been completed already if the feedback was actioned.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1256183637
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
>[INFO] +- org.apache.curator:curator-client:jar:5.2.0:compile
[INFO] | \- com.google.guava:guava:jar:27.0.1-jre:compile
So that module is now using Guava 27.0.1 (instead of 32.0.1 as before), which presumably doesn't have the thing needed by ErrorProne, which normally uses a newer version.
Maven is picking the older version during resolution, probably because it is earlier on the dependency set than the ErrorProne dep is (it will be last due to being in a profile). Its doing that instead of just using the specified dependencyManagement guava version that was removed from the build. Restoring that may fix things. Alternative perhaps there is a newer curator-client version that uses a recent guava.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255783192
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
Right...Guava isnt there, but is is meant to be there. Note its dependencies still being present.
The exclusion I mentioned earlier, in the repackaging the module build does, removed the version the original war included. You have removed the dependency that lead to its replacement in the repackaged war.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1255503083
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
This was here to override the version of guava in the repackaged hawtio war for the console. See the exclusion further down that is still present. Think we will need to check what happens to the management console here, its possible this may have broken it, but at least removed the version override.
##########
pom.xml:
##########
@@ -1097,6 +1094,13 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -Xep:BanJNDI:OFF -XepExcludedPaths:.*/generated-sources/.*</arg>
</compilerArgs>
+ <annotationProcessorPaths>
+ <path>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_core</artifactId>
+ <version>${errorprone.version}</version>
+ </path>
+ </annotationProcessorPaths>
Review Comment:
I know that Emmanuel deliberately avoided doing this when overhauling the ErrorProne usage a while back for JDK > 9, as it restricts use of annotation processors from the classpath and requires every processor be specified here in the XML. We do have another annotation processor being used via the classpath (artemis-log-annotation-processor).
I would guess that is what caused the 'full test run' CI failures @clebertsuconic noted since I know that build enables ErrorProne during compile, meaning it probably then failed to run the other annotation processor as it isnt also specified here (or rather outwith these profiles), and so failed to generate the logger classes and then tripped over their absence.
Since ErrorProne is already isolated to a profile I think I would leave this as it was. At least I dont think it really belongs in the same commit as a swap of caching to use Caffeine.
##########
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java:
##########
@@ -380,7 +380,7 @@ private static String formatByte(byte bite) {
}
private static String formatCase(String string) {
- return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, string);
+ return CaseUtils.toCamelCase(string, false, ' ');
Review Comment:
This seems off? If its going from _UPPER_UNDERSCORE_ its unlikely to have space delimiters..seems more like it will end up with _lower_underscore_ after this? Or does the original also not do what it seems like its trying?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624158604
any idea how active is Caffeine? more specifically how good are them with fixing CVEs?
there was a couple of CVEs in the past with guava. https://www.cvedetails.com/product/52274/Google-Guava.html?vendor_id=1224 and we had to update the version before.
my concern with the replacement is how good would be fix things like these?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#issuecomment-1624485692
apparently there are a lot of test failures if running this. leave it with me for another day please so I can check if it was a real failure or a build issue.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4540:
URL: https://github.com/apache/activemq-artemis/pull/4540#discussion_r1270500400
##########
artemis-hawtio/artemis-console/pom.xml:
##########
@@ -55,18 +55,6 @@
<version>${hawtio.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <!-- License: Apache 2.0 -->
- <!-- see https://github.com/google/guava/issues/3006: checker-compat-qual could be excluded -->
- <exclusions>
- <exclusion>
- <groupId>org.checkerframework</groupId>
- <artifactId>checker-compat-qual</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
Review Comment:
Right, the original war doesnt have it (but does have guava), so it was also excluded from the repackaged war to match. The original war has checker-qual-3.5.0.jar, which should presumably have been excluded when unpacking it, due to the newer guava bringing in checker-qual-3.12.0.jar
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #4540: ARTEMIS-4349 Replace Guava cache with Caffeine
URL: https://github.com/apache/activemq-artemis/pull/4540
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org