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

Added hash_get macro to allow getting multiple hash fields at once #100

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

Conversation

slavak
Copy link
Collaborator

@slavak slavak commented Sep 29, 2020

No description provided.

@slavak slavak requested a review from gavrie September 29, 2020 15:47
Copy link
Contributor

@gavrie gavrie left a comment

Choose a reason for hiding this comment

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

First of all, very impressive macro-fu!

I think it's a bit too much, though. The API using a macro is not very idiomatic; providing a slice-based API would be more user friendly.

I propose a different way to implement this in #101. That's not a full solution though; you can use parts of the macro implementation here to fill out that one.

@slavak
Copy link
Collaborator Author

slavak commented Sep 29, 2020

I agree that a slice-based API would be vastly better. The main reasons I chose not to go with that are:

  1. I think the variadic call convention better communicates the limitations of the API. Accepting a slice implies a generality we can't support with the underlying C API.
  2. While both are limited in the number of fields they can accept, the macro will fail at compile-time when this is exceeded, while the function will fail unexpectedly at runtime, and not even consistently at that. (Since users could reasonably use the API with slices of dynamic length.)

We could theoretically solve both by using a function with macro expansion to utilize a single API call for up to a specified number of fields, and then batch the calls for slices longer than that. That means hiding multiple API calls within the function, though, which could result in performance characteristics that would be surprising to callers.

@gavrie
Copy link
Contributor

gavrie commented Sep 30, 2020

@slavak

  1. I think the variadic call convention better communicates the limitations of the API. Accepting a slice implies a generality we can't support with the underlying C API.

That's a good point.

  1. While both are limited in the number of fields they can accept, the macro will fail at compile-time when this is exceeded, while the function will fail unexpectedly at runtime, and not even consistently at that. (Since users could reasonably use the API with slices of dynamic length.)

Indeed.

We could theoretically solve both by using a function with macro expansion to utilize a single API call for up to a specified number of fields, and then batch the calls for slices longer than that. That means hiding multiple API calls within the function, though, which could result in performance characteristics that would be surprising to callers.

I agree, that's overdoing it.

Bottom line is that we have two principles that seem to clash:

  1. Doing things statically is better than dynamically.
  2. Use friendly APIs are better than cryptic ones.

To resolve this, the best solution would probably be solving this in the Redis modules API. Since this module is intended for a future Redis Enterprise release, that should not be a problem at all. We can simply implement @oranagra's solution and be done with it.

Another thing we can do, both with and without the modules API change, is use serde to deserialize directly into a struct instead of using a HashMap. This has the combined advantages of being fully static and avoiding a HashMap altogether, and being extremely usable since it's a de facto Rust standard. Without the modules API change we can hide all the macro ugliness behind this interface so the user isn't exposed to it. With the change, we can clean up the implementation and keep the interface.

WDYT?

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.

2 participants