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

Use AsyncCache for RefreshingAddressResolver #3547

Open
ikhoon opened this issue May 11, 2021 · 3 comments
Open

Use AsyncCache for RefreshingAddressResolver #3547

ikhoon opened this issue May 11, 2021 · 3 comments

Comments

@ikhoon
Copy link
Contributor

ikhoon commented May 11, 2021

RefreshingAddressResolver caches a DNS result with CompletableFuture. The result is expired by a custom scheduler.
The implementation of async resolving and expiration in RefreshingAddressResolver is pretty complex.

final long nextDelayMillis = refreshBackoff.nextDelayMillis(numAttemptsSoFar++);
if (nextDelayMillis < 0) {
cache.invalidate(hostName);
return null;
}
scheduleRefresh(nextDelayMillis);

It'd better delegate the expiration to Caffeine's AsyncCache for separation of concerns.

@kojilin
Copy link
Contributor

kojilin commented May 15, 2021

There are 2 concerns IMO.

  1. Current implementation re-query even there is no request. If we want to preserve this behavior, we may need to wait Variable refresh ben-manes/caffeine#504 implemented. Or we can change behavior to evict the entry which is not queried during cache time.
  2. Current implementation support caffeine spec, if we want to implement using AsyncCache, I think we will use Expiry in https://github.com/ben-manes/caffeine/wiki/Eviction#time-based because we want to have different TTL for different entry. But it means some of the attributes from spec will conflict with Expiry(e.g. throws exception when user sets something conflict with Expiry. https://github.com/ben-manes/caffeine/blob/b58dc1ca8f6f734fb60779c594d96149b93aed18/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java#L662-L669).

So maybe create new Resolver and let user can choose?

@ben-manes
Copy link

ben-manes commented May 15, 2021

Thanks for the insight @kojilin. I figured there had to be a good reason when mentioning AsyncCache on #3530, but it was unclear (and I am unfamiliar with the code). I think maybe this issue should change the resolver to include a small doc comment about the design intent. Right now it reads as if the code was migrated from Guava's cache and didn't realize there was a simpler async option to further tidy up with.

I don't think variable refresh is a difficult feature to implement, but I probably won't get around to it soon. I think a doc comment would satisfy my review comment and close this ticket.

@kojilin
Copy link
Contributor

kojilin commented May 15, 2021

I'm also have interest in using AsyncCache. But just have concern about behavior change and if there is some latency increase in less frequent use of host after change. So if it's tolerable I will ^^.

The original design looks like cache as much as possible & try to refresh automatically even there is no usage. And if we have Expiry with variable refresh, we won't have such latency increase problem for frequently used host I think. (even dns query is quite fast, if we can refresh during TTL, we will make many request waiting for first query after TTL(?)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants