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

fix:parseComplexKV with unknown metadatatype:7 #427

Closed
wants to merge 2 commits into from

Conversation

fishery86
Copy link
Contributor

kRedisSortedint metadata type 7 out of range

kRedisSortedint metadata type 7 out of range
@@ -54,7 +54,7 @@ Status Parser::parseSimpleKV(const Slice &ns_key, const Slice &value, int expire

Status Parser::parseComplexKV(const Slice &ns_key, const Metadata &metadata) {
RedisType type = metadata.Type();
if (type < kRedisHash || type > kRedisBitmap) {
if (type < kRedisHash || type > kRedisSortedint) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Sortedint is a new addition to Kvrocks and is not supported by Redis, we don't need to deal with it here.

Copy link
Member

@git-hulk git-hulk Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caipengbo Seems we have converted the sorted int to the Redis set type, so it should make sense here?

Copy link
Contributor Author

@fishery86 fishery86 Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a configuration should be added to support such compatiable convert?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is possible, we should support to convert the sorted int to the Redis set type, we shouldn't let user to handle this problem.

Copy link
Member

@ShooterIT ShooterIT Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is possible

this also means user could read sorted int by redis commands after migrating from kvrocks to redis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, @karelrooted should have converted to the Redis set if my memory serves. Can u help to confirm? @caipengbo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find it converted to Redis set.

@caipengbo
Copy link
Contributor

Currently, sorted int is not converted to Redis set type.

@fishery86 Are you interested in supporting this compatibility? If not, I'll support it in the future.

@fishery86
Copy link
Contributor Author

Currently, sorted int is not converted to Redis set type.

@fishery86 Are you interested in supporting this compatibility? If not, I'll support it in the future.

ok,I will do the job

@ShooterIT
Copy link
Member

resolved in #431

@ShooterIT ShooterIT closed this Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants