You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/15 17:47:27 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #6325: GEODE-9155: change frequency of passive expiration
nonbinaryprogrammer opened a new pull request #6325:
URL: https://github.com/apache/geode/pull/6325
increase the time between checks for keys to expire. this reduces the
cpu utilization but causes longer collections when they do collect.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618721962
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -169,8 +169,9 @@ public void testExpire() {
@Test
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates("hash", (k) -> {
- jedis.expire(k, PassiveExpirationManager.INTERVAL);
- GeodeAwaitility.await().during(Duration.ofSeconds(3)).until(() -> true);
+ jedis.expire(k, 1);
+ GeodeAwaitility.await().during(Duration.ofSeconds(PassiveExpirationManager.INTERVAL))
+ .until(() -> true);
Review comment:
Is there a reason we're not really checking that the key expires here?
We could, for example check the following:
```
GeodeAwaitility.await()
.atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL + 1))
.until(() -> jedis.keys("hash").isEmpty());
```
...To make sure the key has actually expired before verifying the keyspace hits and misses. I added 1 minute to the interval to avoid flakiness if passive expiration just ran. Using `atMost` will also allow the test to move on if the condition evaluates to true before the specified duration. The `KEYS` command does not update keyspace hits/misses, so we can safely use that to make sure the key has expired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on pull request #6325:
URL: https://github.com/apache/geode/pull/6325#issuecomment-821309723
I've been thinking about the way we check out a branch in CI and apply the changes. I think, rather than trying to edit the patch file manually, the smarter thing to do may be the following:
1. Check out the same branch locally as the branch we check out in `execute_redis_tests.sh` (right now we do `git checkout origin/5.0`, but we should consider checking out a specific release tag vs. a branch, since that 5.0 branch is still being updated and changes to the tests might mess up our patch [lines might be different, etc.])
2. Apply the existing patch file
3. Make desired changes
4. Generate a new patch file based on those changes
5. Replace the current patch file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r615105398
##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -162,17 +162,33 @@ index de24eabed..aeeb1da7f 100644
test {PERSIST returns 0 against non existing or non volatile keys} {
r set x foo
-@@ -154,39 +154,39 @@ start_server {tags {"expire"}} {
- set size1 [r dbsize]
- # Redis expires random keys ten times every second so we are
- # fairly sure that all the three keys should be evicted after
+@@ -149,44 +149,44 @@ start_server {tags {"expire"}} {
+- test {Redis should actively expire keys incrementally} {
+- r flushdb
+- r psetex key1 500 a
+- r psetex key2 500 a
+- r psetex key3 500 a
+- set size1 [r dbsize]
+- # Redis expires random keys ten times every second so we are
+- # fairly sure that all the three keys should be evicted after
- # one second.
- after 1000
-+ # two seconds.
-+ after 2000
- set size2 [r dbsize]
- list $size1 $size2
- } {3 0}
+- set size2 [r dbsize]
+- list $size1 $size2
+- } {3 0}
++# test {Redis should actively expire keys incrementally} {
++# r flushdb
++# r psetex key1 500 a
++# r psetex key2 500 a
++# r psetex key3 500 a
++# set size1 [r dbsize]
++# # Redis expires random keys ten times every second so we are
++# # fairly sure that all the three keys should be evicted after
++# # one second.
++# after 1000
Review comment:
Is there any value in verifying the keys have expired after 3 minutes and 1 second (181,000 milliseconds) [adding a 1 second buffer in case passive expiration just ran]? The tests only take about 9 minutes in CI right now, so adding an additional 3 minutes would not be a burden. Maybe it's not necessary though since we have this test:
```
@Test
public void should_passivelyExpireKeys() {
jedis.sadd("key", "value");
jedis.pexpire("key", 100);
GeodeAwaitility.await().until(() -> jedis.keys("key").isEmpty());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer merged pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #6325:
URL: https://github.com/apache/geode/pull/6325
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 edited a comment on pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 edited a comment on pull request #6325:
URL: https://github.com/apache/geode/pull/6325#issuecomment-821309723
I've been thinking about the way we check out a branch in CI and apply the changes. I think, rather than trying to edit the patch file manually, it might be easier to do the following:
1. Check out the same branch locally as the branch we check out in `execute_redis_tests.sh` (right now we do `git checkout origin/5.0`, but we should consider checking out a specific release tag vs. a branch, since that 5.0 branch is still being updated and changes to the tests might mess up our patch [lines might be different, etc.])
2. Apply the existing patch file
3. Make desired changes
4. Generate a new patch file based on those changes
5. Replace the current patch file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618624714
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -168,7 +169,7 @@ public void testExpire() {
@Test
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates("hash", (k) -> {
- jedis.expire(k, 1);
+ jedis.expire(k, PassiveExpirationManager.INTERVAL);
GeodeAwaitility.await().during(Duration.ofSeconds(3)).until(() -> true);
});
}
Review comment:
It seems like the intention of this test is to ensure keyspace hits and misses don't update after the `EXPIRE` command and after the key passively expires. We can leave the time we set the key to expire as 1 second (`jedis.expire(k, 1);`). However, we need to increase the time we are waiting for the key to expire passively since that passive expiration interval has changed.
Perhaps we could change the await to be something like this:
```
GeodeAwaitility.await()
.atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL))
.until(() -> jedis.keys("hash").isEmpty());
```
...so we're verifying the key expires before checking the keyspace hits and misses. We will need to add some time to that INTERVAL in the await for a buffer and to avoid potential flakiness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618498253
##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -162,17 +162,33 @@ index de24eabed..aeeb1da7f 100644
test {PERSIST returns 0 against non existing or non volatile keys} {
r set x foo
-@@ -154,39 +154,39 @@ start_server {tags {"expire"}} {
- set size1 [r dbsize]
- # Redis expires random keys ten times every second so we are
- # fairly sure that all the three keys should be evicted after
+@@ -149,44 +149,44 @@ start_server {tags {"expire"}} {
+- test {Redis should actively expire keys incrementally} {
+- r flushdb
+- r psetex key1 500 a
+- r psetex key2 500 a
+- r psetex key3 500 a
+- set size1 [r dbsize]
+- # Redis expires random keys ten times every second so we are
+- # fairly sure that all the three keys should be evicted after
- # one second.
- after 1000
-+ # two seconds.
-+ after 2000
- set size2 [r dbsize]
- list $size1 $size2
- } {3 0}
+- set size2 [r dbsize]
+- list $size1 $size2
+- } {3 0}
++# test {Redis should actively expire keys incrementally} {
++# r flushdb
++# r psetex key1 500 a
++# r psetex key2 500 a
++# r psetex key3 500 a
++# set size1 [r dbsize]
++# # Redis expires random keys ten times every second so we are
++# # fairly sure that all the three keys should be evicted after
++# # one second.
++# after 1000
Review comment:
Exposing PassiveExpirationManager.INTERVAL done; I think the other test obviates the need for the TCL test. We're explicitly deviating from Redis behavior, so not worrying about their integration tests in this scenario seems okay. Or have I misunderstood the question?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618624714
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -168,7 +169,7 @@ public void testExpire() {
@Test
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates("hash", (k) -> {
- jedis.expire(k, 1);
+ jedis.expire(k, PassiveExpirationManager.INTERVAL);
GeodeAwaitility.await().during(Duration.ofSeconds(3)).until(() -> true);
});
}
Review comment:
It seems like the intention of this test is to ensure keyspace hits and misses don't update after the `EXPIRE` command and after the key passively expires. We can leave the time we set the key to expire as 1 second (`jedis.expire(k, 1);`). However, we need to increase the time we are waiting for the key to expire passively since that passive expiration interval has changed.
Perhaps we could change the await to be something like this:
```
GeodeAwaitility.await()
.atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL))
.until(() -> jedis.keys("hash").isEmpty());
```
...so we're verifying the key expires before checking the keyspace hits and misses. We will need to add some time in the await to that INTERVAL for a buffer and to avoid potential flakiness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618646851
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -168,7 +169,7 @@ public void testExpire() {
@Test
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates("hash", (k) -> {
- jedis.expire(k, 1);
+ jedis.expire(k, PassiveExpirationManager.INTERVAL);
GeodeAwaitility.await().during(Duration.ofSeconds(3)).until(() -> true);
});
}
Review comment:
Yeah, I see what you mean now. I'll fix it up!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618620813
##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -162,17 +162,33 @@ index de24eabed..aeeb1da7f 100644
test {PERSIST returns 0 against non existing or non volatile keys} {
r set x foo
-@@ -154,39 +154,39 @@ start_server {tags {"expire"}} {
- set size1 [r dbsize]
- # Redis expires random keys ten times every second so we are
- # fairly sure that all the three keys should be evicted after
+@@ -149,44 +149,44 @@ start_server {tags {"expire"}} {
+- test {Redis should actively expire keys incrementally} {
+- r flushdb
+- r psetex key1 500 a
+- r psetex key2 500 a
+- r psetex key3 500 a
+- set size1 [r dbsize]
+- # Redis expires random keys ten times every second so we are
+- # fairly sure that all the three keys should be evicted after
- # one second.
- after 1000
-+ # two seconds.
-+ after 2000
- set size2 [r dbsize]
- list $size1 $size2
- } {3 0}
+- set size2 [r dbsize]
+- list $size1 $size2
+- } {3 0}
++# test {Redis should actively expire keys incrementally} {
++# r flushdb
++# r psetex key1 500 a
++# r psetex key2 500 a
++# r psetex key3 500 a
++# set size1 [r dbsize]
++# # Redis expires random keys ten times every second so we are
++# # fairly sure that all the three keys should be evicted after
++# # one second.
++# after 1000
Review comment:
Yeah, that definitely makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 edited a comment on pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 edited a comment on pull request #6325:
URL: https://github.com/apache/geode/pull/6325#issuecomment-821309723
I've been thinking about the way we check out a branch in CI and apply the changes. Rather than trying to edit the patch file manually, it might be easier to do the following:
1. Check out the same branch locally as the branch we check out in `execute_redis_tests.sh` (right now we do `git checkout origin/5.0`, but we should consider checking out a specific release tag vs. a branch, since that 5.0 branch is still being updated and changes to the tests might mess up our patch [lines might be different, etc.])
2. Apply the existing patch file
3. Make desired changes
4. Generate a new patch file based on those changes
5. Replace the current patch file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r618721962
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -169,8 +169,9 @@ public void testExpire() {
@Test
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates("hash", (k) -> {
- jedis.expire(k, PassiveExpirationManager.INTERVAL);
- GeodeAwaitility.await().during(Duration.ofSeconds(3)).until(() -> true);
+ jedis.expire(k, 1);
+ GeodeAwaitility.await().during(Duration.ofSeconds(PassiveExpirationManager.INTERVAL))
+ .until(() -> true);
Review comment:
`Duration.ofSeconds` should be `Duration.ofMinutes`.
Is there a reason we're not really checking that the key expires here?
We could, for example check the following:
```
GeodeAwaitility.await()
.atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL + 1))
.until(() -> jedis.keys("hash").isEmpty());
```
...To make sure the key has actually expired before verifying the keyspace hits and misses. I added 1 minute to the interval to avoid flakiness if passive expiration just ran. Using `atMost` will also allow the test to move on if the condition evaluates to true before the specified duration. The `KEYS` command does not update keyspace hits/misses, so we can safely use that to make sure the key has expired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org