You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/24 03:53:12 UTC
[GitHub] [iceberg] kbendick opened a new pull request #3803: Update caffeine cache library to latest 2.x release
kbendick opened a new pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803
The caffeine cache library version we use is somewhat old (May 2020) and since I've been looking through it, I thought I'd take a chance to update it.
The release notes can be found here: https://github.com/ben-manes/caffeine/releases
There are 9 releases between 2.8.4 and 2.9.3. The releases before 2.9.0 are all small (but possibly important) bug fixes.
The 2.x release line is still being maintained for now. The 3.x line was introduced due to a breaking API change that would only affect users who depend on this `Cache` to have the exact same interfaces as Guava and some other caching libraries.
Here's the incompatibilities between 2.x and 3.x (which are mostly due to the need to support people who use this cache in place of Guava or similar libraries):
```
Incompatible changes
VarExpiration time-based puts now return the old value instead of a boolean result
Removed jandex resource as no longer utilized by Quarkus
Split Policy.Expiration into fixed and refresh interfaces
```
I played it safe and stayed on 2.x, but I personally think we should just go to 3.x. The differences between the two (in the changelog) are the same, outside of 3.0.0 and 2.9.0, where 3.0.0 has the incompatible parts as well as a few extra perf enhancements.
Some of the relevant fixes for us might include:
From 2.8.5:
- Fixed expiration delay for scheduled cleanup [#431](https://github.com/ben-manes/caffeine/issues/431), which might have helped out with the potential deadlock that is seen in #3791, as the reported issue is very similar to #3791 (though I think we should still use the fix provided there)
From 2.9.0:
- Added Caffeine.evictionListener which is notified within the atomic operation when an entry is automatically removed. (This method was how I was originally going to attempt to fix #3791 until @racevedoo graciously supplied a fix in #3801)
- Added triggering cache maintenance if an iterator observes an expired entry for more aggressive eviction
If people want to upgrade to 3.x, I can do that too. It seems not only safe, but actually safer than our current code on 2.8.4.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] racevedoo commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
racevedoo commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1006974012
> cc @racevedoo.
>
> Also, maybe this change should wait until after the release, like with https://github.com/apache/iceberg/pull/3815?
This looks good to me, but I guess it can wait until after the release
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick edited a comment on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick edited a comment on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000637247
Some of the relevant fixes for us might include:
From 2.8.5:
- Fixed expiration delay for scheduled cleanup [#431](https://github.com/ben-manes/caffeine/issues/431), which might have helped out with the potential deadlock reported by @racevedoo, as the reported issue is very similar to #3791 (though I think we should still use the fix that they provided for now if it's been verified against the tables that they can consistently see the deadlock on).
From 2.9.0:
- Added Caffeine.evictionListener which is notified within the atomic operation when an entry is automatically removed. (This method was how I was originally going to attempt to fix #3791 until @racevedoo graciously supplied a fix in #3801)
- Added triggering cache maintenance if an iterator observes an expired entry for more aggressive eviction
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000637247
Some of the relevant fixes for us might include:
From 2.8.5:
- Fixed expiration delay for scheduled cleanup [#431](https://github.com/ben-manes/caffeine/issues/431), which might have helped out with the potential deadlock that is seen in #3791, as the reported issue is very similar to #3791 (though I think we should still use the fix provided there)
From 2.9.0:
- Added Caffeine.evictionListener which is notified within the atomic operation when an entry is automatically removed. (This method was how I was originally going to attempt to fix #3791 until @racevedoo graciously supplied a fix in #3801)
- Added triggering cache maintenance if an iterator observes an expired entry for more aggressive eviction
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick removed a comment on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick removed a comment on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000655553
I'm going to wait until https://github.com/apache/iceberg/pull/3801 is merged in and then follow up on 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick edited a comment on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick edited a comment on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000680057
I'm going to wait until #3801 is merged in and then follow up on this. This should be considered a draft, but I can't convert it to draft now AFAIK.
Caffeine is very nuanced, and there's a change required in #3801 (for tests only) that's not required by upgrading to 2.8.5 from 2.8.4.
Given the complexity of `Caffeine` and the time I've spent looking into it, I'm going to help keep our eyes on keeping this dependency up to date etc.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000655553
I'm going to wait until https://github.com/apache/iceberg/pull/3801 is merged in and then follow up on 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000680057
I'm going to wait until #3801 is merged in and then follow up on this.
Caffeine is very nuanced, and there's a change required in #3801 (for tests only) that's not required by upgrading to 2.8.5 from 2.8.4.
Given the complexity of `Caffeine` and the time I've spent looking into it, I'm going to help keep our eyes on keeping this dependency up to date etc.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: [DRAFT - Do not merge yet] Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000950819
> The main incompatibility was internal by requiring Java 11 so that Unsafe could be replaced by VarHandles. Otherwise it was api cleanups and implementation improvements that warranted waiting for a major release. It is good to keep up to date as it is very complex code so there have unfortunately been bugs despite my best efforts.
>
> You might consider using this plugin to help track updates: https://github.com/ben-manes/gradle-versions-plugin
Anything that complex is likely to see some bugs now and again. But it’s worth the complexity as it’s quite efficient!
So the 3.x line requires Java 11? I missed that. Thanks for the clarification. Due to Hive compatibility and compatibility with the various engines we integrate with, we currently build for / with Java 8.
Thanks for the info!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000637444
Here's the incompatibilities between 2.x and 3.x (which are mostly due to the need to support people who use this cache in place of Guava or similar libraries). It's a rather small list.
```
Incompatible changes
VarExpiration time-based puts now return the old value instead of a boolean result
Removed jandex resource as no longer utilized by Quarkus
Split Policy.Expiration into fixed and refresh interfaces
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ben-manes commented on pull request #3803: [DRAFT - Do not merge yet] Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
ben-manes commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1000699960
The main incompatibility was internal by requiring Java 11 so that Unsafe could be replaced by VarHandles. Otherwise it was api cleanups and implementation improvements that warranted waiting for a major release. It is good to keep up to date as it is very complex code so there have unfortunately been bugs despite my best efforts.
You might consider using this plugin to help track updates:
https://github.com/ben-manes/gradle-versions-plugin
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3803: Update caffeine cache library to latest 2.x release
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3803:
URL: https://github.com/apache/iceberg/pull/3803#issuecomment-1006957232
cc @racevedoo.
Also, maybe this change should wait until after the release, like with https://github.com/apache/iceberg/pull/3815?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org