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

[FEATURE] Request ReplyUPtr expose at Redis++ Async response #554

Open
lisay-yan opened this issue Mar 27, 2024 · 4 comments
Open

[FEATURE] Request ReplyUPtr expose at Redis++ Async response #554

lisay-yan opened this issue Mar 27, 2024 · 4 comments

Comments

@lisay-yan
Copy link

Is your feature request related to a problem? Please describe.
My use case prefer send all kinds of raw command, such as lua etc, and prefer to parse hiredis redisReply by application itself.

Describe the solution you'd like
Make ReplyUPtr = std::unique_ptr<redisReply, ReplyDeleter> possible to access as Async cluster client as Sync client did now.
Then, application can parse redisReply per itself.

Describe alternatives you've considered
N/A

Additional context
I prefer to know if that is feasible, and if doable, the draft schedule. Thanks.

@lisay-yan
Copy link
Author

Hi, Dear code officers

Due to this request block my current project, so, I did simple changes to expose hiredis redisReply.
Would you like support review and approval? Thanks so much.

#555

For sure, we will keep the mindset, redisReply after CB, it will be freed by hiredis. THanks.

@sewenew
Copy link
Owner

sewenew commented Mar 31, 2024

Thanks for your PR! However, it's not a good idea to parse the reply to redisReply*. There's an override command interface that returns RedisUPtr, however, it might not fit for async interface. As you mentioned, redisReply is freed by hiredis framework. If you expose redisReply * to end user, they might use it incorrectly, especially the life scope of the raw pointer.

Why not use the generic interface to do the parse work? It works well with most cases, including Lua scripting. If you compile the code with C++17, with the power of std::variant, you can handle all cases of Redis reply.

Regards

@lisay-yan
Copy link
Author

@sewenew

Most of traffic in my use case is redisModule, and user already with business logic to decode redisReply in current system.
To integrate with Reds-Plus-Plus only for cluster aware, so, if user can guard safe to use redisReply, expose it maybe a good choice for user which doesn't start from zero?? Agree?
And My application can't be C++17 for some reason now.

Since my PR had been draft ready, is it possible to gain your approval to provide an option to Redis-Plus-Plus user at least??
Thanks for understanding.

@sewenew
Copy link
Owner

sewenew commented Apr 8, 2024

Instead of exposing redisReply to user, I was planing to expose a ReplyParser interface. So that user can parse redisReply to a user defined data structure. That might solve your problem. However, I'm too busy to start the project.

Regards

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

2 participants