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

Generalization of .getNanosToWaitForReset() #187

Closed
helins opened this issue Nov 9, 2021 · 7 comments
Closed

Generalization of .getNanosToWaitForReset() #187

helins opened this issue Nov 9, 2021 · 7 comments

Comments

@helins
Copy link

@helins helins commented Nov 9, 2021

Hi, really good library, thanks a lot for your work!

I had to use the 7.0.0-beta-2020-04-18 release only because the ConsumptionProbe has this method. I am using Caffeine as a local cache for buckets and need this information to compute accurate TTLs on buckets every time tokens are consumed. By getting this full refill interval, a bucket can be safely marked as stale after this point in time: we know it will be full, so there is no point keeping it in memory.

Some questions / comments:

  1. There isn't any other way getting this information in the stable release, is there? Using the beta is mandatory?
  2. I need to apply the same logic when adding tokens manually but it is a bit absurd: refill what is needed + force refill 1 token + consume 1 token to get the ConsumptionProbe ; It would be useful obtaining this information (.getNantosToWaitForReset()) outside of consumption probes.
@vladimir-bukhtoyarov
Copy link
Collaborator

@vladimir-bukhtoyarov vladimir-bukhtoyarov commented Nov 9, 2021

Hello @helins

  1. It is not hard for me to backport this functionality.
  2. I did not understand the second question. Could you explain with more details what are you trying to achieve? Also, did you consider using estimateAbilityToConsume?

@helins
Copy link
Author

@helins helins commented Nov 13, 2021

  1. Thanks, that would be awesome since the beta seems to be quite re-organized and non-trivial :)

  2. So, simply put, I need to know the remaining nanos to wait for reset when I .addTokens as well. In an ideal world, .addTokens would return ConsumptionProbe as well (or similar). This would allow setting an appropriate TTL on buckets not only when tokens are consumed but also when they are manually refilled.

@vladimir-bukhtoyarov
Copy link
Collaborator

@vladimir-bukhtoyarov vladimir-bukhtoyarov commented Nov 13, 2021

ok, now I understand your use case.

In an ideal world, .addTokens would return ConsumptionProbe as well (or similar).

Different users want to add different additional information for API methods. For example:

  • Add current configuration to result of tryConsumeAndReturnRemaining #117
  • Add bandwidth ID that prevents consuming tokens #185
  • Add currently available tokens per each bandwidth as the result of any consumption operation #189

It is impossible to cover all these feature requests, but fortunately, I have created the universal API that covers such requests without any library modifications. It is VerboseApi and its asynchronous analog.
Examples of usage of this API can be found in the following answers:

It is not hard for me to backport this functionality.

Since I have a better understanding of your requirements, I would prefer to add method getNanosToWaitForReset() to the VerboseResult instead of extending ConsumtionProbe.

@vladimir-bukhtoyarov
Copy link
Collaborator

@vladimir-bukhtoyarov vladimir-bukhtoyarov commented Nov 14, 2021

With version 6.4.0 desired behavior can be achieved in the following way:

VerboseResult<Nothing> result = bucket.asVerbose().addTokens(1);
long waitForRefillNanos = result.getDiagnostics().calculateFullRefillingTime();

@helins
Copy link
Author

@helins helins commented Nov 19, 2021

Ah, it was looking promising, exactly what I needed, unfortunately:

java.lang.IllegalArgumentException: Can't call public method of non-public class: public abstract long io.github.bucket4j.VerboseResult$Diagnostics.calculateFullRefillingTime()

Indeed, the Diagnostics interface is only package accessible.

vladimir-bukhtoyarov added a commit that referenced this issue Nov 20, 2021
@vladimir-bukhtoyarov
Copy link
Collaborator

@vladimir-bukhtoyarov vladimir-bukhtoyarov commented Nov 20, 2021

Ahh, I forgot to mark the interface as public. Fixed in version 4.6.1

@helins
Copy link
Author

@helins helins commented Nov 20, 2021

Thanks for your prompt action, works perfectly and does the job :)

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

No branches or pull requests

2 participants