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

feat(server): SCAN command add ATTR options #4766

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lichuang
Copy link
Contributor

feat(server): SCAN command add ATTR options

fix #4537

@lichuang
Copy link
Contributor Author

lichuang commented Mar 14, 2025

@romange

for (3), (4) we need to introduce an additional flag in CompactObject to track it on keys (note that usually the flags keys are set on values).

There is TOUCHED in CompactObject, is it can be used for (3) (4)?

Another question, i want to add some python test cases, but redis.Redis scan function has only these options:

SCAN cursor [MATCH pattern] [COUNT count] [TYPE type]

(see: https://redis.io/docs/latest/commands/scan/)

@romange
Copy link
Collaborator

romange commented Mar 19, 2025

Yes, good point. We mark the keys and do it irrespective of tiering, so it should work well for this issue.
We will also need unit tests to test this behaviour.

Regarding the testing - I believe this falls well into the unit test territory, I would add the tests to generic_family_test (it is possible to use pytest redis client but I think it's just an overkill)

vec = StrArray(resp.GetVec()[1]);
EXPECT_EQ(2, vec.size());

ASSERT_THAT(Run({"get", "foo"}), "bar");
Copy link
Contributor Author

@lichuang lichuang Mar 23, 2025

Choose a reason for hiding this comment

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

@romange In this test case, i found that running "get" of some key, it wiil not make the key touched, so this test case failed, can you have a look what is wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it makes the key touched and you can verify it by adding debug printings for example at line db_slice.cc:624

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it makes the key touched and you can verify it by adding debug printings for example at line db_slice.cc:624

i see what is wrong, HasExpire() works on it->second object(which is value), while HasTouched() works on it->first object(which is key), is that expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, this by design though I agree it's confusing.
the reason is that if you call "SADD foo bar" and then SET foo bar then all the attributes on the value (second) will be reset. if we want to preserve the attributes we should use keys (first)

@romange
Copy link
Collaborator

romange commented Mar 30, 2025

@lichuang a friendly ping :)
you almost finished, just checking if you are planning to address the comments :)

@lichuang
Copy link
Contributor Author

@lichuang a friendly ping :) you almost finished, just checking if you are planning to address the comments :)

sorry, i am a little busy these days, i will finish it ASAP.

@lichuang lichuang marked this pull request as ready for review March 31, 2025 15:11
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.

allow scanning based on keys attributes
2 participants