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

ThreadlocalRandom instead of ThreadLocal #367

Open
MartinWitt opened this issue Oct 19, 2023 · 8 comments
Open

ThreadlocalRandom instead of ThreadLocal #367

MartinWitt opened this issue Oct 19, 2023 · 8 comments
Labels
P4 type=other Miscellaneous activities not covered by other type= labels

Comments

@MartinWitt
Copy link

Hey,

While reading the changes in the newest releases and the code for it, I saw this comment. https://github.com/google/flogger/blob/master/api/src/main/java/com/google/common/flogger/SamplingRateLimiter.java#L55 If I understand the build system correct flogger requires at least java 8, so ThreadLocalRandom(https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html) or ThreadLocal.withInitial is available. Is the comment a relict of the past and the code could be changed?

@hagbard
Copy link
Contributor

hagbard commented Oct 20, 2023

Flogger is JDK 7 currently.
https://github.com/google/flogger/blob/master/api/BUILD#L42
which is exactly why I had to avoid using ThreadLocalRandom when I wrote that code.

@MartinWitt
Copy link
Author

@hagbard
Copy link
Contributor

hagbard commented Oct 20, 2023

Weirdly not, despite the initial JavaDoc saying so (many methods are individually listed as 1.8).

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html

Basically I originally wrote it with TLR and it didn't compile, so I switched to doing it manually.

If someone can get it working (including Android) with TLR I'm fine with it, but it didn't work when I tried it.

I don't think it's a big deal though, but thanks for suggesting it.

David

@cpovirk
Copy link
Member

cpovirk commented Oct 20, 2023

The limitation is very likely Android.

@hagbard
Copy link
Contributor

hagbard commented Oct 20, 2023

I guess there's a chance I started writing this bit of API before Flogger bumped to 1.7 and I'm misremembering which version was causing the issue. It's been internally available in Google for a while now.

@kluever kluever added P4 type=other Miscellaneous activities not covered by other type= labels labels Oct 20, 2023
@MartinWitt
Copy link
Author

I can provide a PR refactoring this, if this doesn't create more work for you. Is there some android CI?

@hagbard
Copy link
Contributor

hagbard commented Oct 23, 2023

The Android work is internal to Google and not open sourced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

No branches or pull requests

4 participants