-
Notifications
You must be signed in to change notification settings - Fork 883
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
chore: implemente User and UserRegistry classes #1693
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for reviewing while not being asked for, just strolling around 😊
src/server/acl/user.h
Outdated
inline constexpr uint32_t ACL_CATEGORY_KEYSPACE = 1ULL << 0; | ||
inline constexpr uint32_t ACL_CATEGORY_READ = 1ULL << 1; | ||
inline constexpr uint32_t ACL_CATEGORY_WRITE = 1ULL << 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you have a dedicated namespace with global constants, reminds me of an.... enum (class) ! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but enum class does not support implicit conversions between objects and the underlying type which it would make it a nightmare to use without casting on each operation. The other idea was enum + namespace which now that I am thinking it it's a good fit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a typed enum like enum class AclCategory : unit32_t { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but this requires explicit conversions it's used. class enums protect against it.
for example see https://godbolt.org/z/xvK5MMb4v
src/server/acl/user.h
Outdated
// when optional is empty, the special `nopass` password is implied | ||
// password hashed with xx64 | ||
std::optional<uint64_t> password_; | ||
uint32_t acl_categories_{0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe = ACL_CATEGORY_NONE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%
src/server/acl/user_registry.h
Outdated
// TODO add abseil mutex attributes | ||
mutable absl::Mutex mu_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use fiber compatible mutexes (we have a Mutex
in the dfly namespace re-exported from helio) to block only a single fiber instead of blocking the full thread. However most of you operations are really quick, Roman likes to use spinlocks in this case. I don't have any real data or benchmarks to judge with confidence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do that, although spinlock would be kinda worst for operations require a bit more computation (like filtering against the key-space for example, which is not a priority atm but still I want this to be easily extended in the future)
you are always welcome 👨🍳 |
{"HASH", AclCat::ACL_CATEGORY_HASH}, | ||
{"STRING", AclCat::ACL_CATEGORY_STRING}, | ||
{"BITMAP", AclCat::ACL_CATEGORY_BITMAP}, | ||
{"HYPERLOG", AclCat::ACL_CATEGORY_HYPERLOGLOG}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be HYPERLOGLOG?
This PR implements the basic functionality required to build the ACL commands.