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

Attribute type #20

Open
BridgeAR opened this issue Feb 5, 2019 · 3 comments
Open

Attribute type #20

BridgeAR opened this issue Feb 5, 2019 · 3 comments

Comments

@BridgeAR
Copy link

BridgeAR commented Feb 5, 2019

I just went ahead and implemented RESP3 for the Node.js redis parser and I struggle to see the how this can be delivered to the user in a useful way.

It is not clear if the attribute alone should be delivered to the user or if it should be delivered in combination with the actual corresponding data (which IMHO would make sense). The latter however requires the parsing logic to become more complex in cases for aggregated data types such as arrays, maps and sets.

It is also not clear when exactly the data should be delivered as in: immediately after the attribute is parsed or after the top level is done parsing? (This is again about aggregated data types)

Would it not be better to just provide the very same information as push information that just contains the actual data on top of the attribute? It's of course extra data to parse but it would simplify the parser logic. Right now the attributes somewhat seem out of line for me compared to the rest of the spec.

@AngusP
Copy link
Contributor

AngusP commented Jul 3, 2019

It may perhaps be a good idea to stipulate that the attributes won't ever contain information that you really shouldn't chuck away and can't get another way so that simpler libraries can exist that ignore it completely without missing something really important.

For the higher-level languages like Python or JS, it might be clearer to return a response as a class structure that contains the attributes, rather than working out some way of representing them with native datatypes like dicts (so response.items, response.attributes and response.items[0]['somekey'].attributes, as examples)

@BridgeAR
Copy link
Author

BridgeAR commented Feb 6, 2020

@AngusP that would work but it does not sound like a good solution to me. In most cases the data type would be the plain response, so wrapping the return type requires more memory, slows down the parser due to creating more objects and requires to check for the attribute property for all returned values.

@antirez it would be great if you could have a look at this. Is there a real need for the attribute type? It seems very difficult to implement this in a useful way.

@mgravell
Copy link
Contributor

mgravell commented Feb 13, 2020

In particular, it is very awkward for nested data; most clients would be able to add some kind of "and the attributes associated with the last payload are ...", as per the key popularity example shown in the spec (r.attribs) - but it is not obvious how to efficiently do this for sub-data, like the {ttl:3600} example in the spec

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

No branches or pull requests

3 participants