Release 2.5.1 #1622
Release 2.5.1 #1622
Conversation
The check allow we passing -1 is wrong, without this fix, passing a -1 will crash the server.
This is a minor fix, SCRIPT EXISTS requires at least three arguments. Before the change, we can do this: ``` 127.0.0.1:6666> script exists (empty array) ``` After: we will return the wrong number of arguments error.
The old code called ResetWatchedKeys before the error, and then ResetWatchedKeys reset the watched_keys_modified flag which causes the following inconsistencies (with Redis). kvrocks output: ``` 127.0.0.1:6666> watch a OK 127.0.0.1:6666> watch b ->>> other client touch a OK 127.0.0.1:6666> exec (error) ERR EXEC without MULTI 127.0.0.1:6666> multi OK 127.0.0.1:6666(TX)> ping QUEUED 127.0.0.1:6666(TX)> exec 1) PONG ``` redis output: ``` 127.0.0.1:6379> watch a OK 127.0.0.1:6379> watch b ->>> other client touch a OK 127.0.0.1:6379> exec (error) ERR EXEC without MULTI 127.0.0.1:6379> multi OK 127.0.0.1:6379(TX)> ping QUEUED 127.0.0.1:6379(TX)> exec (nil) ```` Introduced in apache#1279
…-existing key (apache#1573) The current code GETHASH returns an error for a key that doesn't exist: ``` 127.0.0.1:6666> geohash mykey a b c (error) ERR NotFound: ``` In Redis, this would return: ``` 127.0.0.1:6379> geohash mykey a b c 1) (nil) 2) (nil) 3) (nil) ``` GEOPOS also has the same issue, this PR fixes these inconsistencies in this case.
Note that this may break compatibility since in the past doing: `DECRBY key -9223372036854775808` would succeed but create an invalid result. And now we will return an error rejecting the request. Before: ``` 127.0.0.1:6666> set key 0 OK 127.0.0.1:6666> decrby key -9223372036854775807 (integer) 9223372036854775807 127.0.0.1:6666> get key "9223372036854775807" 127.0.0.1:6666> set key 0 OK 127.0.0.1:6666> decrby key -9223372036854775808 (integer) -9223372036854775808 127.0.0.1:6666> get key "-9223372036854775808" ``` After: ``` 127.0.0.1:6666> decrby key -9223372036854775808 (error) ERR decrement would overflow ```
before: ``` 127.0.0.1:6666> zunion 1 zset (error) ERR wrong number of arguments ``` after: ``` 127.0.0.1:6666> zunion 1 zset (empty array) 127.0.0.1:6666> zunion 1 zset 1) "a" 2) "b" 3) "c" ```
ZRANGESTORE arity should be -5, the code wrongtly sets it to -4, when passing 4 arguments, server will trigger a Segmentation fault and result a crash: ``` 127.0.0.1:6666> zrangestore dstzset srczset 0 Error: Server closed the connection not connected> ```
Co-authored-by: git-hulk <hulk.website@gmail.com> Co-authored-by: Twice <twice@apache.org>
… logs (apache#1556) This PR has two parts. The first one, internally, both XREAD and XRANGE call the Stream::Range function. When Range fails, we should return an error. The second one is to remove the debug logs. These logs were there in the earliest days, I guess they are for debugging, remove them now to avoid polluting the server logs. The blpop one is easy to reproduce: ``` Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value ```
The name should be used_memory_rss_human according to the context and Redis documentation.
RocksDB PORTABLE was set to 0 after apache#1516 and it may return an illegal instruction error when running on the AMD platform. This PR fixes this issue by changing the default value of PORTABLE to 1 so that it can compile the rocksdb without platform special instructions.
This doesn't matter, just a minor fix, zremrangebyscore doesn't accept extra arguments.
When the code is doing `store`, it calls the add method, which leads us to perform zadd dst xxx in disguise. And this result the following two issues. The first one is we do a type check against dst: ``` 127.0.0.1:6379> set dst value OK 127.0.0.1:6379> zrangestore dst non 0 -1 (integer) 0 127.0.0.1:6379> get dst (nil) 127.0.0.1:6666> set dst value OK 127.0.0.1:6666> zrangestore dst non 0 -1 (error) ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value ``` The second one is when dst has members, we are doing zadd: ``` 127.0.0.1:6379> zadd dst 1 a 2 b (integer) 2 127.0.0.1:6379> zadd src 3 c 4 d (integer) 2 127.0.0.1:6379> zrangestore dst src 0 -1 (integer) 2 127.0.0.1:6379> zrange dst 0 -1 1) "c" 2) "d" 127.0.0.1:6666> zadd dst 1 a 2 b (integer) 2 127.0.0.1:6666> zadd src 3 c 4 d (integer) 2 127.0.0.1:6666> zrangestore dst src 0 -1 (integer) 2 127.0.0.1:6666> zrange dst 0 -1 1) "a" 2) "b" 3) "c" 4) "d" ``` ZRANGESTORE was added in apache#1482
Co-authored-by: Twice <twice@apache.org>
git-hulk
requested review from
PragmaTwice,
torwig,
enjoy-binbin,
ShooterIT,
tisonkun and
caipengbo
I think you're the best bug hunter in our community for sure. |
|
Seems that VERSION.txt is not changed. |
|
And I think squash merge is not a good idea for this PR. But I don't know how to change it. So maybe you can just push these commits rather than merge the PR. |
Yes, I think we can propose commits and get approved first, then merge them into 2.5 and append the version commit. |
ummm... Looks like it has no way to enable the rebase button for some special branches only: https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#Git.asf.yamlfeatures-Mergebuttons. Let me pick those commits and push them to 2.5 first, you guys can help to revise them later. |
|
in the repo's settings, maybe it is https://github.com/apache/kvrocks/settings i think we've done it pretty well before, like #468 |
|
@enjoy-binbin The merge/rebase button is controlled by: https://github.com/apache/kvrocks/blob/unstable/.asf.yaml#L32 and it seems no way to open for some branches only. |
|
ohh, in this way, maybe we can also enable the merge button? i think we can go beyond some PRs. |
|
Yes, I think we can enable the merge button and the merger should know when to use the merge button.
I will re-edit the PR summary later. |
In some cases, such as a release version, we will pick some commits, then do a PR review, and then merge the commits into the branch. In this case, we want to use plain merge to keep the info of each commit instead of using squash-merge. Or in some refactoring PRs, we can separate code moves (rename) and actual changes into different commits. These single commits we want to keep the commit message and doing a plain merge. In this way, when tracking changes, we can only focus on actual changes without being affected by meaningless diffs. When enabled, we can control it (when we are doing merge, usually squash is used, and in some cases we need to keep commits we use merge). That is, the choice of squash or merge is the responsibility of the person for merging the PR. See apache#1622 for more discussion on release part.
|
i raise a PR to enable the plain merge in #1624, we can move the discussion there |
|
we need to update the VERSION.txt as well |
@enjoy-binbin Thanks for your warm reminder. The package source will change and change version and I plan to do it when creating the release tarball. But it's also good to append the version change now. |
|
i see the tag we made is v2.5.1, we used to tag it without v. see https://github.com/apache/kvrocks/releases but in here it is 2.5.1, so https://github.com/apache/kvrocks/releases/tag/2.5.1 is a 404 link |
|
I think it is a bug of GitHub. Our release tag is It is not required to be equal. No problem here for ourselves. |
Highlights: #1568 may the server crash when passing numkeys is -1 in Lua script, and #1615 is caused by the data race when updating the slots info string in cluster mode. That means it won't affect you if you're not using the Lua script or cluster mode.
Bug Fixes