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

Bump up caffeine dependencies #549

Conversation

eschizoid
Copy link
Contributor

@eschizoid eschizoid commented May 7, 2020

The purpose of this PR is to add support for the AsyncCache and not only theAsyncLoadingCache

@eschizoid eschizoid force-pushed the feature/bump-up-caffeine-dependency branch from 3fed3ff to 49ba50d Compare May 7, 2020 20:00
@eschizoid
Copy link
Contributor Author

hey @checketts wondering if you can give it a quick look to this little guy, thanks!

@brian-brazil
Copy link
Contributor

We purposefully list old versions so that we don't inadvertently break backwards compatibility.

@eschizoid
Copy link
Contributor Author

eschizoid commented May 7, 2020

We purposefully list old versions so that we don't inadvertently break backwards compatibility.

hey @brian-brazil thanks for your quick reply. Is there any other way we can add support for this new type of caching that caffeine added on recent versions?

This upgrade should not break previous implementations since now both classes share a common interface, hope that helps :)

P.S. I ran the tests and they passed, please let me know if there is any other validation I could do.

@brian-brazil
Copy link
Contributor

So if someone was running 2.3.0, then this wouldn't break them?

@eschizoid
Copy link
Contributor Author

eschizoid commented May 8, 2020

hey @brian-brazil attached my answers.

So if someone was running 2.3.0, then this wouldn't break them?

Thats is correct, given the following interface:

package com.github.benmanes.caffeine.cache;
...
public interface AsyncLoadingCache<K, V> extends AsyncCache<K, V> {
...
}

Caffeine now offers some other methods that allow you to have an instance of AsyncCache

Let me know what you think :)

@brian-brazil
Copy link
Contributor

I'm not convinced, there is no AsyncCache in 2.3.0.

@eschizoid
Copy link
Contributor Author

eschizoid commented May 8, 2020

I'm not convinced, there is no AsyncCache in 2.3.0.

correct, so folks that remain on version 0.8.1 won't have problems

Now let's say we merge this PR and folks migrate to version 0.8.2, these folks won't have to change their working code since AsyncCache is the base class for AsyncLoadingCache . Their code will remain as it is, other than having to bump up the dependency.

so folks can do any of this without breaking the existing API:

collector.addCache("existing_cache_implementation", AsyncLoadingCache)
collector.addCache("base_class_implementation", AsyncCache)

the test cases are already proving that exiting code will continue to work.

Would it be helpful if I add some examples on the REAMDE file explaining how to migrate to the new version and take advantage of java inheritance?

If you feel more comfortable we can also do this:

public void addCache(String cacheName, AsyncLoadincCache cache) {
    children.put(cacheName, cache.synchronous());
}

public void addCache(String cacheName, AsyncCache cache) {
    children.put(cacheName, cache.synchronous());
}

I feel is a little bit redundant given what I just explained, please let me know what you think :)

@brian-brazil
Copy link
Contributor

correct, so folks that remain on version 0.8.1 won't have problems

But folks who upgrade to the next version of client_java would. I don't like forcing users to upgrade to new versions of dependencies unless there's a non-trivial gain in terms of the metrics available. Especially when the new version hasn't even been out two weeks yet.

@eschizoid
Copy link
Contributor Author

eschizoid commented May 8, 2020

I don't like forcing users to upgrade to new versions of dependencies unless there's a non-trivial gain in terms of the metrics available.

I see. I think the java client will get some benefits on the metric side since it will now support the two types of caches that caffeine offers.

Especially when the new version hasn't even been out two weeks yet.

That is fair, I should have been more careful with my suggestion. What about if we bump up to the version where the interface AsyncCache was introduced?

https://github.com/ben-manes/caffeine/blob/v2.7.0/caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java

This artifact was released to maven central (Feb 24, 2019)

@brian-brazil Let me know what you think about this more cautious approach :)

@brian-brazil
Copy link
Contributor

That sounds more reasonable.

@eschizoid eschizoid force-pushed the feature/bump-up-caffeine-dependency branch from 49ba50d to 7bd13ea Compare May 8, 2020 20:33
@eschizoid
Copy link
Contributor Author

eschizoid commented May 8, 2020

That sounds more reasonable.

@brian-brazil Done :)

Let me know what you think

Signed-off-by: Mariano Gonzalez <mariano@otus.com>
@eschizoid eschizoid force-pushed the feature/bump-up-caffeine-dependency branch from 7bd13ea to 67dac33 Compare May 8, 2020 20:35
@brian-brazil brian-brazil merged commit b3b9790 into prometheus:master May 9, 2020
@brian-brazil
Copy link
Contributor

Thanks!

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.

None yet

2 participants