Add support for `com.github.ben-manes.caffeine:caffeine:2.9.3` by linghengqian · Pull Request #388 · oracle/graalvm-reachability-metadata

linghengqian · 2023-09-09T13:11:43Z

What does this PR do?

Fixes Add support for com.github.ben-manes.caffeine:caffeine:2.9.3 #384 .
Add support for com.github.ben-manes.caffeine:caffeine:2.9.3.
According to my test at linghengqian-scratches/graalvm-trace-metadata-smoketest@2f29acb and linghengqian-scratches/graalvm-trace-metadata-smoketest@15ef51e, the test results of the APIs of 2.9.3 and 3.1.2 are quite inconsistent.
This PR is related to the Error detected at Make ShardingSphere Proxy in GraalVM Native Image form available apache/shardingsphere#21347 .

Code sections where the PR accesses files, network, docker or some external service

(example link to code section)
Null.

Checklist before merging

I have considered including reachability metadata directly in the library or the framework (see our contributing document)
I am the original author of all content provided in the pull request, and I did not copy the content from any other source (see the tests section from our contributing document)
For all tests where I am not the sole author, I have added a comment that proves I may publish them under the specified license (see the tests section from our contributing document)
I have properly formatted metadata files (see our formatting guide)
I have added thorough tests (see the tests section from our contributing document)
I have filled all places where my pull request accesses files, network, docker, or any other external service (see the section above)

linghengqian

An useless JSON was removed at linghengqian-scratches/graalvm-trace-metadata-smoketest@f1ec6a2 . However, if we remove the following JSON entries involving java.**, it will cause nativeTest to hang, so it must be retained. I haven't seen room for improvement yet.

{
    "condition":{"typeReachable":"com.github.benmanes.caffeine.cache.StripedBuffer"},
    "name":"java.lang.Thread",
    "fields":[{"name":"threadLocalRandomProbe"}]
  }

dnestoro · 2024-02-12T13:21:52Z

I will review this PR in the coming weeks.
Since we had issues on previous PRs @wirthi and @olpaw should also have a look at this one.

wirthi · 2024-02-12T14:30:07Z

I will review this PR in the coming weeks. Since we had issues on previous PRs @wirthi and @olpaw should also have a look at this one.

I am a bit concerned about the WriteBehindCacheWriter class https://github.com/oracle/graalvm-reachability-metadata/pull/388/files#diff-3e57a3d8b35bdd4b113214b87578827af5752e0f36a4a5d7e00ffffa8e8adc44 - This seems to be a direct copy of https://github.com/ben-manes/caffeine/blob/v2.9.3/examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava/WriteBehindCacheWriter.java published under Apache license there.

linghengqian · 2024-02-12T15:50:33Z

I will review this PR in the coming weeks. Since we had issues on previous PRs @wirthi and @olpaw should also have a look at this one.

I am a bit concerned about the WriteBehindCacheWriter class https://github.com/oracle/graalvm-reachability-metadata/pull/388/files#diff-3e57a3d8b35bdd4b113214b87578827af5752e0f36a4a5d7e00ffffa8e8adc44 - This seems to be a direct copy of https://github.com/ben-manes/caffeine/blob/v2.9.3/examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava/WriteBehindCacheWriter.java published under Apache license there.

This is actually a tool class that is easily deduced from the documentation, see https://github.com/ben-manes/caffeine/wiki/Compute . Scenarios outside of RxJava are generally not thought of.

A write-back extension might implement some or all of the following features:
Batching and coalescing the operations
Delaying the operations until a time window
Performing a batch prior to a periodic flush if it exceeds a threshold size
Loading from the write-behind buffer if the operations have not yet been flushed
Handling retrials, rate limiting, and concurrency depending on the the characteristics of the external resource
See write-behind-rxjava for a simple example using RxJava.

This PR uses io.reactivex.rxjava3:rxjava:3.1.6 instead of the corresponding io.reactivex.rxjava2:rxjava:2.1.12 in https://github.com/ben-manes/caffeine/blob/v2.9.3/examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava/WriteBehindCacheWriter.java . They are not equal.
Do you think the unit tests here still need to be removed?

linghengqian · 2024-02-12T16:29:03Z

I will review this PR in the coming weeks. Since we had issues on previous PRs @wirthi and @olpaw should also have a look at this one.

I am a bit concerned about the WriteBehindCacheWriter class https://github.com/oracle/graalvm-reachability-metadata/pull/388/files#diff-3e57a3d8b35bdd4b113214b87578827af5752e0f36a4a5d7e00ffffa8e8adc44 - This seems to be a direct copy of https://github.com/ben-manes/caffeine/blob/v2.9.3/examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava/WriteBehindCacheWriter.java published under Apache license there.

This is actually a tool class that is easily deduced from the documentation, see https://github.com/ben-manes/caffeine/wiki/Compute . Scenarios outside of RxJava are generally not thought of.

A write-back extension might implement some or all of the following features:
Batching and coalescing the operations
Delaying the operations until a time window
Performing a batch prior to a periodic flush if it exceeds a threshold size
Loading from the write-behind buffer if the operations have not yet been flushed
Handling retrials, rate limiting, and concurrency depending on the the characteristics of the external resource
See write-behind-rxjava for a simple example using RxJava.

This PR uses io.reactivex.rxjava3:rxjava:3.1.6 instead of the corresponding io.reactivex.rxjava2:rxjava:2.1.12 in https://github.com/ben-manes/caffeine/blob/v2.9.3/examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava/WriteBehindCacheWriter.java . They are not equal.

Do you think the unit tests here still need to be removed?

The unit tests corresponding to Compute have been removed.

wirthi · 2024-02-14T14:08:53Z

This is actually a tool class that is easily deduced from the documentation, see https://github.com/ben-manes/caffeine/wiki/Compute . Scenarios outside of RxJava are generally not thought of.

Thanks for the explanation. An argument might be that this is trivial code that anyone would write like this (using the documentation quoted by you). However, this was ~30 LOC, so this argument legally will be on very thin ice. A judge would maybe see that you copied the file from somewhere, removed the comments, and changed the license. We could not accept a contribution like that.

The unit tests corresponding to Compute have been removed.

Ok, thanks. In the remaining code, I see no problem any more. I assume all the remaining tests were written by yourself?

linghengqian · 2024-02-14T14:47:33Z

This is actually a tool class that is easily deduced from the documentation, see https://github.com/ben-manes/caffeine/wiki/Compute . Scenarios outside of RxJava are generally not thought of.

Thanks for the explanation. An argument might be that this is trivial code that anyone would write like this (using the documentation quoted by you). However, this was ~30 LOC, so this argument legally will be on very thin ice. A judge would maybe see that you copied the file from somewhere, removed the comments, and changed the license. We could not accept a contribution like that.

The unit tests corresponding to Compute have been removed.

Ok, thanks. In the remaining code, I see no problem any more. I assume all the remaining tests were written by yourself?

This PR involves unit tests from my extraction and organization. Refer to linghengqian-scratches/graalvm-trace-metadata-smoketest@f70e8bc#diff-e7d1df473b724c8c5c457302a50f2461abdaea72e1e53911502f8c9d6de01338 . That git was originally used to test the use of Graalvm Native Image under WSL2. I don't think the LICENSE issue involved in this PR requires additional handling in another git.

wirthi · 2024-02-14T14:53:47Z

It is fine if you already contributed the code somewhere else; if it is YOUR code, that YOU wrote, you can also contribute it here. That's what I assume is the case here. If you can confirm that, I think this is resolved.

@dnestoro will still check the functionality of the PR as usual

Thanks for improving this.

dnestoro · 2024-02-20T11:56:06Z

@linghengqian in general the PR looks fine. For future PRs make sure that you only ever add the minimal amount tests needed to verify the metadata your PR adds.

The use of tests in the metadata repository is not to cover all possible features of the library but to exercise the code paths in the library that wouldn't work without the metadata your PR adds.

olpaw

LGTM, but please address all issues/questions raised by @dnestoro before we can merge.

linghengqian

All reviews have been processed.

linghengqian mentioned this pull request Sep 9, 2023

Make ShardingSphere Proxy in GraalVM Native Image form available apache/shardingsphere#21347

Closed

7 tasks

linghengqian marked this pull request as ready for review September 9, 2023 13:25

linghengqian requested a review from a team as a code owner September 9, 2023 13:25

linghengqian requested a review from vjovanov September 9, 2023 13:25

linghengqian mentioned this pull request Sep 9, 2023

ClassNotFoundException with Caffeine and Spring Boot 3 #271

Closed

vjovanov requested a review from dnestoro September 19, 2023 11:37

linghengqian force-pushed the fixture-caffeine branch from 455d961 to 2c46b03 Compare September 21, 2023 17:57

linghengqian commented Sep 21, 2023

View reviewed changes

linghengqian force-pushed the fixture-caffeine branch from 2c46b03 to 143c4bd Compare December 5, 2023 11:37

linghengqian force-pushed the fixture-caffeine branch from 143c4bd to b12db26 Compare January 18, 2024 15:53

linghengqian force-pushed the fixture-caffeine branch from b12db26 to 86c3fae Compare February 11, 2024 11:07

linghengqian mentioned this pull request Feb 12, 2024

Add support for org.apache.zookeeper:zookeeper:3.8.1 #204

Closed

linghengqian force-pushed the fixture-caffeine branch from 86c3fae to 13ebb50 Compare February 12, 2024 16:25