You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "weim0000 (via GitHub)" <gi...@apache.org> on 2024/12/02 09:11:40 UTC
[PR] fix(string):reset the value of expired key for setrange cmd [kvrocks]
weim0000 opened a new pull request, #2686:
URL: https://github.com/apache/kvrocks/pull/2686
reset the value of expired key to prevent the following parsing and calculation.
`127.0.0.1:6003> set k v
OK
127.0.0.1:6003> expire k 1
(integer) 1
after 2 seconds
127.0.0.1:6003> get k
(nil)
127.0.0.1:6003> setrange k 1 aa
(integer) 10
127.0.0.1:6003> get k
(nil)
127.0.0.1:6003> exists k
(integer) 0
127.0.0.1:6003>`
1 set value with expire time.
2 wait until the key expires
3 use setrange to this key. The cmd be successful, but get k return nil
**What did you expect to see?**
`127.0.0.1:6003> set k v
OK
127.0.0.1:6003> expire k 1
(integer) 1
127.0.0.1:6003> get k
(nil)
127.0.0.1:6003> setrange k 0 aa
(integer) 10
127.0.0.1:6003> get k
aa
127.0.0.1:6003> exists k
(integer) 1
127.0.0.1:6003>`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865750829
##########
src/types/redis_string.cc:
##########
@@ -297,6 +297,7 @@ rocksdb::Status String::SetRange(engine::Context &ctx, const std::string &user_k
}
Metadata metadata(kRedisString, false);
+ raw_value.clear();
Review Comment:
@git-hulk Valid point. Since the value has expired, we should return "nothing" (empty) to the caller of `getRawValue`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2513558394
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [61.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865731369
##########
src/types/redis_string.cc:
##########
@@ -297,6 +297,7 @@ rocksdb::Status String::SetRange(engine::Context &ctx, const std::string &user_k
}
Metadata metadata(kRedisString, false);
+ raw_value.clear();
Review Comment:
I think it'd be better to clear the raw_value inside `getRawValue` if the return status isn't ok. @torwig @PragmaTwice @mapleFU What do you think?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865715128
##########
tests/cppunit/types/string_test.cc:
##########
@@ -250,6 +250,15 @@ TEST_F(RedisStringTest, SetRange) {
string_->Get(*ctx_, key_, &value);
EXPECT_EQ(16, value.size());
auto s = string_->Del(*ctx_, key_);
+
+ string_->SetEX(*ctx_, key_, "test-value", 1);
+ sleep(2);
Review Comment:
@git-hulk It's a C++ test not Go :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865721364
##########
tests/cppunit/types/string_test.cc:
##########
@@ -250,6 +250,15 @@ TEST_F(RedisStringTest, SetRange) {
string_->Get(*ctx_, key_, &value);
EXPECT_EQ(16, value.size());
auto s = string_->Del(*ctx_, key_);
+
+ string_->SetEX(*ctx_, key_, "test-value", 1);
+ sleep(2);
Review Comment:
My bad 😢
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2511200787
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [66.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2511446217
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2686&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2686&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [66.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2686&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2686)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1866210764
##########
src/types/redis_string.cc:
##########
@@ -297,6 +297,7 @@ rocksdb::Status String::SetRange(engine::Context &ctx, const std::string &user_k
}
Metadata metadata(kRedisString, false);
+ raw_value.clear();
Review Comment:
Good to me. Actually refactor them to StatusOr can solve the problem, but I don't have much time to do that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "weim0000 (via GitHub)" <gi...@apache.org>.
weim0000 commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2511038796
> @weim0000 Thanks for your fix, would you mind adding a test case to cover this? https://github.com/apache/kvrocks/blob/unstable/tests/gocase/unit/type/strings/strings_test.go
Ok, no problem. I will add the go test cases ASAP
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865731369
##########
src/types/redis_string.cc:
##########
@@ -297,6 +297,7 @@ rocksdb::Status String::SetRange(engine::Context &ctx, const std::string &user_k
}
Metadata metadata(kRedisString, false);
+ raw_value.clear();
Review Comment:
I think it'd be better to clear the raw_value inside `getRawValue` if the return status isn't ok. Because we call `getRawValue` many times and it's hard to keep having an eye on this. @torwig @PragmaTwice @mapleFU What do you think?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2511012732
@weim0000 Thanks for your fix, would you mind adding a test case to cover this? https://github.com/apache/kvrocks/blob/unstable/tests/gocase/unit/type/strings/strings_test.go
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#discussion_r1865707664
##########
tests/cppunit/types/string_test.cc:
##########
@@ -250,6 +250,15 @@ TEST_F(RedisStringTest, SetRange) {
string_->Get(*ctx_, key_, &value);
EXPECT_EQ(16, value.size());
auto s = string_->Del(*ctx_, key_);
+
+ string_->SetEX(*ctx_, key_, "test-value", 1);
+ sleep(2);
Review Comment:
```suggestion
time.Sleep(2 * time.Second);
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(string): reset the value of expired key for SETRANGE cmd [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2686:
URL: https://github.com/apache/kvrocks/pull/2686#issuecomment-2513763816
@weim0000 Thanks for your fix.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org