Skip to content

Conversation

@kirillovsky
Copy link
Contributor

Hi! Looks like you use Guava only for caching. Did you consider to migrate from Guava to Caffeine?
Caffeine advantages:

More other it's reduce Opa-Kafka-Plugin fat-jar size from 2.9MB to 917Kb

@anderseknert
Copy link
Contributor

Thanks! I'm really out of the loop around most things JVM these days, but it seems reasonable to me. @scholzj, wdyt?

@scholzj
Copy link
Collaborator

scholzj commented Apr 7, 2025

I think originally, the use of Guava was good because Kafka already used Guava in the first place. However, that is not the case anymore and Kafka seems to be using Caffeine now as well (not sure what exactly they use it for and if the motivation for the change was the same as suggested here or completely different).

So I think moving to Caffeine might make sense. But as the Kafka 4.0 and latest main branch are using Caffeine 3.1, it would be good to understand if it works with that as well (I have no idea what the differences are between Caffeine 3.1 and 3.2 and if they are backward compatible - avoiding multiple versions on the classpath always makes things easier).

build.gradle Outdated
!(it.moduleGroup in [
'org.openpolicyagent.kafka',
'com.google.guava'
'com.google.guava',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if we don't use it anymore, there is no reason to list it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be some white space changes here which will screw up the Git history. Can we keep only the real code changes and remove the whitespace changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed please check

@anderseknert
Copy link
Contributor

@kirillovsky LGTM, but could you please revert the whitespace changes so that we can merge this? Thanks!

@kirillovsky kirillovsky force-pushed the migrate-cache-to-caffeine branch from 41fe094 to b032af1 Compare April 15, 2025 06:27
@kirillovsky
Copy link
Contributor Author

@anderseknert @scholzj Thanks for checking my PR. Fixed

@kirillovsky
Copy link
Contributor Author

kirillovsky commented Apr 15, 2025

@anderseknert @scholzj Regarding useless whitespaces in my PR. It's happens case my IDEA automatically changes Line separator from LF (POSIX style) to CRLF (OS Windows style). But there is a way to avoid it automatically (via .gitattributes) - https://docs.github.com/en/get-started/git-basics/configuring-git-to-handle-line-endings#per-repository-settings. Can i add it to your repo? What do you think?

@kirillovsky kirillovsky force-pushed the migrate-cache-to-caffeine branch from b032af1 to 27b94d2 Compare April 15, 2025 06:50
@kirillovsky kirillovsky force-pushed the migrate-cache-to-caffeine branch from 27b94d2 to 2320fe1 Compare April 15, 2025 06:52
Copy link
Contributor

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! The majority of changes still look style related though? If you want to break up long lines or fix style issues, it's always better to do so in a PR only submitted for that purpose, as combining a change in behavior/semantics and style changes makes reviewing much more difficult. But that aside, looks good to me, and @scholzj is you agree, go ahead and merge.

Copy link
Collaborator

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

@scholzj scholzj merged commit 5f45709 into StyraOSS:main Apr 15, 2025
@scholzj
Copy link
Collaborator

scholzj commented Apr 15, 2025

Sure! The majority of changes still look style related though? If you want to break up long lines or fix style issues, it's always better to do so in a PR only submitted for that purpose, as combining a change in behavior/semantics and style changes makes reviewing much more difficult. But that aside, looks good to me, and @scholzj is you agree, go ahead and merge.

Yeah, I agree that in general it is best to do such changes in a separate PR to have clearer history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants