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

RedisRateLimiter#calcRemainingQuota should use opsForValue instead of boundValueOps #108

Closed
lchayoun opened this issue Sep 3, 2018 · 3 comments
Assignees

Comments

@lchayoun
Copy link
Collaborator

lchayoun commented Sep 3, 2018

the code for calcRemainingQuota should be similar to calcRemainingLimit

@marcosbarbero
Copy link
Owner

@lchayoun I just noticed that by using this new approach we are no longer setting the reset value as it was before

You can check here for reference.

Should it be rolled back?

@marcosbarbero
Copy link
Owner

ping @lchayoun

@lchayoun
Copy link
Collaborator Author

Sorry for the delayed response
Since it is not really important, and performance is prefered, I think the code should be as follow:

    @Override
    protected void calcRemainingLimit(Long limit, Long refreshInterval,
                                      Long requestTime, String key, Rate rate) {
        if (limit != null) {
            long usage = requestTime == null ? 1L : 0L;
            rate.setReset(SECONDS.toMillis(refreshInterval));
            Long current = 0L;
            try {
                current = redisTemplate.opsForValue().increment(key, usage);
                // Redis returns 1 when the key is incremented for the first time, and the expiration time is set
                if (current != null && current.equals(1L)) {
                    handleExpiration(key, refreshInterval);
                }
            } catch (RuntimeException e) {
                String msg = "Failed retrieving rate for " + key + ", will return limit";
                rateLimiterErrorHandler.handleError(msg, e);
            }
            rate.setRemaining(Math.max(-1, limit - current));
        }
    }

    private void handleExpiration(String key, Long refreshInterval) {
        try {
            this.redisTemplate.expire(key, refreshInterval, SECONDS);
        } catch (RuntimeException e) {
            String msg = "Failed setting expiration for " + key;
            rateLimiterErrorHandler.handleError(msg, e);
        }
    }

lchayoun added a commit to SWCE/spring-cloud-zuul-ratelimit that referenced this issue Jul 2, 2019
* Add Spring Cloud Finchley Support [ci deploy]

Add Spring Cloud Finchley support

* marcosbarbero#89 A service with different interfaces has different policies (marcosbarbero#90)

* marcosbarbero#89 A service with different interfaces has different policies

* Get the first IP from X-Forwarded-For list (marcosbarbero#91)

* Get the first IP from X-Forwarded-For list

* Add unit test for multiple x-forwarded-for

* Upgrade version [ci deploy]

* Upgrade version [ci deploy]

* Fix marcosbarbero#92

* Bump version [ci deploy]

* marcosbarbero#97 Fetching username for type USER through a custom implementation - RateLimitUtils should be overridable (marcosbarbero#98)

* [ci deploy] 2.0.3.RELEASE

* [ci deploy] version 2.0.3.RELEASE

* marcosbarbero#99 Properties validation should fail if a policy is configured without limit and without quota (marcosbarbero#100)

* [ci deploy] version 2.0.4.RELEASE

* Update README.adoc

* Redis performance enhancement (marcosbarbero#107)

Fixes marcosbarbero#103

* [ci deploy] new release

* Adds performance improvement for RedisRateLimiter (marcosbarbero#109)

Fixes marcosbarbero#108

* Add License header and minor changes (marcosbarbero#110)

Changes:

  - Add LICENSE Header where it was missing
  - Minor code/style changes

* [ci deploy] new release

* marcosbarbero#112 Remove InMemory implementation (marcosbarbero#113) [ci deploy]

Fixes marcosbarbero#112

* Remove deprecated properties

Fixes marcosbarbero#116

* Next release version 2.2.0 (marcosbarbero#118) [ci deploy]

@lchayoun I'm opening this PR just for a discussion about the next release version.
As we've removed the deprecated properties with marcosbarbero#117 I think it's better if we move from `2.1.0` to `2.2.0`.

Do you agree? Any considerations?

* Update dependencies (marcosbarbero#120)

* Fix tests configurations (marcosbarbero#123)

Fixes the tests configuration removing deprecated properties

* Increase codeco (marcosbarbero#124)

* Increase codeco

* Remove unused import

* Minor refactor

* Minor refactoring (marcosbarbero#125)

* Fix security alerts (marcosbarbero#128)

* Minor refactoring

* Fix security alerts

* Add rate limiting based on user-role evalutation (marcosbarbero#130)

Fixes marcosbarbero#127

* Release 2.2.1.RELEASE [ci deploy]

* Add role type documentation

* Add sample secured application config per user

* Update documentation with role type

* Feature RateLimitType.METHOD (marcosbarbero#136)

Fixes marcosbarbero#135 

#### Changes proposed in this pull request:
 - Add RateLimitType.METHOD
 - Test RateLimitType.METHOD
 - Remove unuesed imports in test case by git commit.

* Bump version [ci deploy]

* spring-cloud-zuul-ratelimit-141 Upgrade Bucket4J and caches versions (marcosbarbero#142)

Fixes marcosbarbero#141 

#### Changes proposed in this pull request:
 - Upgrade Bucket4J version to 4.3.0
 - Upgrade Hazelcast version to 3.11
 - Upgrade Ignite version to 2.7.0
 - Upgrade Infinispan version to 9.4.5

* BUG fix: quota never expire (marcosbarbero#147)

* Add clientId implementation (marcosbarbero#146)

* Add clientId implementation

* Add OAuth2 tests

* Upgrade Spring Cloud and Spring Boot

* Use managed version for spring-security

* Trigger build

* Revert "Add clientId implementation" (marcosbarbero#148)

Reverts marcosbarbero#146

* Custom hide or show header in response (marcosbarbero#150)

#### Changes proposed in this pull request:
 - custom hide or show header in the response

* Release [ci deploy]

* Migrade autoconfig test to spring boot 2

* Refactor support test classes

* Update README.adoc (marcosbarbero#159)[ci skip]

* Fixed issue 160 so that it only applies one limit per request. (marcosbarbero#161)

* Fixed issue 160 so that it only applies one limit per request and adds breakOnMatch parameter.

* Added two unit tests to test basic functionality and an entry in the Policy Properties for breakOnMatch
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

2 participants