You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "GoGim1 (via GitHub)" <gi...@apache.org> on 2023/09/08 04:16:11 UTC
[GitHub] [kvrocks] GoGim1 opened a new pull request, #1747: fix type name of bloom filter
GoGim1 opened a new pull request, #1747:
URL: https://github.com/apache/kvrocks/pull/1747
(no comment)
--
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
[GitHub] [kvrocks] mapleFU commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319347990
##########
src/storage/redis_metadata.h:
##########
@@ -62,7 +62,7 @@ enum RedisCommand {
};
const std::vector<std::string> RedisTypeNames = {"none", "string", "hash", "list", "set",
- "zset", "bitmap", "sortedint", "stream"};
+ "zset", "bitmap", "sortedint", "stream", "MBbloom--"};
Review Comment:
Lets keep consistent with Redis
--
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
[GitHub] [kvrocks] mapleFU commented on pull request #1747: Fix `TYPE` command with bloom filter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#issuecomment-1711066342
Also cc @zncleon
--
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
[GitHub] [kvrocks] mapleFU commented on pull request #1747: Fix `TYPE` command with bloom filter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#issuecomment-1711068924
This patch is LGTM. Would you mind add a tiny test in `tests/gocase/unit/type/bloom/bloom_test.go` for this?
--
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
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319669476
##########
src/storage/redis_metadata.h:
##########
@@ -62,7 +62,7 @@ enum RedisCommand {
};
const std::vector<std::string> RedisTypeNames = {"none", "string", "hash", "list", "set",
- "zset", "bitmap", "sortedint", "stream"};
+ "zset", "bitmap", "sortedint", "stream", "MBbloom--"};
Review Comment:
got it.
--
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
[GitHub] [kvrocks] GoGim1 commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "GoGim1 (via GitHub)" <gi...@apache.org>.
GoGim1 commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319341335
##########
src/storage/redis_metadata.h:
##########
@@ -62,7 +62,7 @@ enum RedisCommand {
};
const std::vector<std::string> RedisTypeNames = {"none", "string", "hash", "list", "set",
- "zset", "bitmap", "sortedint", "stream"};
+ "zset", "bitmap", "sortedint", "stream", "MBbloom--"};
Review Comment:
> https://github.com/RedisBloom/RedisBloom/blob/master/src/rebloom.c#L1448
Redis seems to use `MBbloom--` as the type name of the bloom filter. Should we be consistent?
--
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
[GitHub] [kvrocks] enjoy-binbin commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319811229
##########
src/commands/cmd_key.cc:
##########
@@ -37,6 +37,7 @@ class CommandType : public Commander {
RedisType type = kRedisNone;
auto s = redis.Type(args_[1], &type);
if (s.ok()) {
+ if (type >= RedisTypeNames.size()) return {Status::RedisExecErr, "Invalid type"};
Review Comment:
i think in this case we can trigger a crash instead of returning an error.
returning an error will make the user think he is doing something wrong,
when in fact this is a kvrocks internally bug (user can fire a issue and report this)
but i am also ok with it since mostly it will be a dead code.
please also add a comment near by RedisType and RedisTypeNames, mention that they need to stay in sync
--
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
[GitHub] [kvrocks] GoGim1 commented on pull request #1747: Fix `TYPE` command with bloom filter
Posted by "GoGim1 (via GitHub)" <gi...@apache.org>.
GoGim1 commented on PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#issuecomment-1711129058
> This patch is LGTM. Would you mind add a tiny test in `tests/gocase/unit/type/bloom/bloom_test.go` for this?
done
--
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
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1747: fix type name of bloom filter
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319336461
##########
src/storage/redis_metadata.h:
##########
@@ -62,7 +62,7 @@ enum RedisCommand {
};
const std::vector<std::string> RedisTypeNames = {"none", "string", "hash", "list", "set",
- "zset", "bitmap", "sortedint", "stream"};
+ "zset", "bitmap", "sortedint", "stream", "MBbloom--"};
Review Comment:
```suggestion
"zset", "bitmap", "sortedint", "stream", "bloomfilter"};
```
--
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
[GitHub] [kvrocks] enjoy-binbin commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1319811229
##########
src/commands/cmd_key.cc:
##########
@@ -37,6 +37,7 @@ class CommandType : public Commander {
RedisType type = kRedisNone;
auto s = redis.Type(args_[1], &type);
if (s.ok()) {
+ if (type >= RedisTypeNames.size()) return {Status::RedisExecErr, "Invalid type"};
Review Comment:
i think in this case we can trigger a crash instead of returning an error.
returning an error will make the user think he is doing something wrong,
when in fact this is a kvrocks internally bug (user can fire a issue and report this)
but i am also ok with it since mostly it will be a dead code.
(actually returning nothing seems better than returning an error)
please also add a comment near by RedisType and RedisTypeNames, mention that they need to stay in sync
--
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
[GitHub] [kvrocks] GoGim1 commented on a diff in pull request #1747: Fix `TYPE` command with bloom filter
Posted by "GoGim1 (via GitHub)" <gi...@apache.org>.
GoGim1 commented on code in PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747#discussion_r1320500549
##########
src/commands/cmd_key.cc:
##########
@@ -37,6 +37,7 @@ class CommandType : public Commander {
RedisType type = kRedisNone;
auto s = redis.Type(args_[1], &type);
if (s.ok()) {
+ if (type >= RedisTypeNames.size()) return {Status::RedisExecErr, "Invalid type"};
Review Comment:
done
--
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
[GitHub] [kvrocks] enjoy-binbin merged pull request #1747: Fix `TYPE` command with bloom filter
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin merged PR #1747:
URL: https://github.com/apache/kvrocks/pull/1747
--
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