Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for com.github.ben-manes.caffeine:caffeine:2.9.3 #388

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

linghengqian
Copy link
Contributor

@linghengqian linghengqian commented Sep 9, 2023

What does this PR do?

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

  • (example link to code section)

  • Null.

Checklist before merging

Copy link
Contributor Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

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

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

@dnestoro
Copy link
Member

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
Copy link
Member

wirthi commented Feb 12, 2024

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
Copy link
Contributor Author

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.

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.

@linghengqian
Copy link
Contributor Author

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.

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.

  • The unit tests corresponding to Compute have been removed.

@wirthi
Copy link
Member

wirthi commented Feb 14, 2024

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
Copy link
Contributor Author

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?

@wirthi
Copy link
Member

wirthi commented Feb 14, 2024

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
Copy link
Member

@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 olpaw self-requested a review February 20, 2024 12:35
Copy link
Member

@olpaw olpaw left a comment

Choose a reason for hiding this comment

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

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

@linghengqian linghengqian force-pushed the fixture-caffeine branch 3 times, most recently from 7a956b2 to 700c88a Compare February 21, 2024 04:40
Copy link
Contributor Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

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

  • All reviews have been processed.

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.

Add support for com.github.ben-manes.caffeine:caffeine:2.9.3
4 participants