Skip to content

Conversation

@anarthal
Copy link
Collaborator

Adds config::restore_pubsub_state and command_context
Deprecates the boost_redis_to_bulk extension point taking a std::string& argument, in favor of an extension point with the same name taking a command_context argument

close #367

@anarthal
Copy link
Collaborator Author

@mzimbres this is a prototype of the pubsub state restoration feature. I'd like to get this done before the next release, so we can make async_receive2 not cancel at all during reconnections.

The idea is that, when adding a SUBSCRIBE (or related) command to a request, we keep track of the channels this request is subscribing to (or unsubscribing from). When the request succeeds, we store the channels in a std::set. Every time a reconnection happens, we compose commands to subscribe to the channels the user was using before the reconnection.

This requires inspecting command arguments from request. It's something the current boost_redis_to_bulk extension point makes difficult, since the request is never passed to the function. I've deprecated it in favor of a new extension point, with the same name, but taking a command_context argument. This is a class wrapping request that allows extension points to add arguments to a command. We need to inspect command arguments if we ever intend to support cluster.

This is still missing docs and tests - let me know if you think this is the right approach before spending time on these. Also, I can split the PR into smaller ones (e.g. deprecating the old extension point first) if you think it's best.

sentinel_config sentinel{};

// (TODO: document properly) Do we want to re-subscribe to channels on reconnection?
bool restore_pubsub_state = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be no option to deactivate restoration in the connection. We can offer a request option e.g. request::config::track_subscriptions.


enum class pubsub_change_type;

class pubsub_state {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhpas name it subscriptions or subscription _tracker?


void pubsub_state::commit_changes(const request& req)
{
for (const auto& ch : detail::request_access::pubsub_changes(req))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (const auto& ch : detail::request_access::pubsub_changes(req))
for (const auto& ch : request_access::pubsub_changes(req))

@mzimbres
Copy link
Collaborator

This will be a great improvement over the current complex resubscribe loop, so you have my approval. I don't understand yet why boost_redis_to_bulk is a problem. We can intercept SUBSCRIBE arguments on request::push/range. How idoes boost_redis_to_bulk poses a problem there?

@anarthal
Copy link
Collaborator Author

This will be a great improvement over the current complex resubscribe loop, so you have my approval. I don't understand yet why boost_redis_to_bulk is a problem. We can intercept SUBSCRIBE arguments on request::push/range. How idoes boost_redis_to_bulk poses a problem there?

At the point of push, the argument is a custom user type, not a string. After boost_redis_to_bulk, we have a serialized blob string (that is, length, separator, actual string, separator). To get the actual string value, we would have to parse the serialized blob string. That's not very elegant (or efficient).

@mzimbres
Copy link
Collaborator

At the point of push, the argument is a custom user type, not a string. After boost_redis_to_bulk, we have a serialized blob string (that is, length, separator, actual string, separator). To get the actual string value, we would have to parse the serialized blob string. That's not very elegant (or efficient).

I think we should avoid all this trouble by adding request::subcribe/unsubcribe/... member functions where we can intercept the channels more easily. Parsing the payload to find out the channels sounds defeating, those functions are too generic to work with, it has been the plan since the start to add more specialized request members, which are friendlier to the user.

@anarthal
Copy link
Collaborator Author

This does work for this task, but idk if it does for cluster (I don't have the answer to this question, maybe it does). Cluster needs to know the key each command is acting on and compute its hash to know where to direct the command to.

We could support some syntax like

request.push_command(subscribe{"hello"});

Although you probably need a regular and a range version of the command.

@mzimbres
Copy link
Collaborator

We could support some syntax like
request.push_command(subscribe{"hello"});
Although you probably need a regular and a range version of the command.

This looks interesting because with reflection we could perhaps generate these structs automatically from a description doc (I would call it push2). I am wondering whether the two steps to call push2 wouldn't hurt usability: first construct subscribe than push it.

@anarthal
Copy link
Collaborator Author

ATM reflection can only generate aggregates. I lack the vision to know if that'd be enough (it wouldn't if we need a templated range, for instance).

It'd much better for usability having functions in request, actually. But then request is gonna end up having zounds of functions, and not all of them used frequently.

@anarthal
Copy link
Collaborator Author

How about

req.push2(subscribe, {"channel", "channel2"});

Where subscribe is an object of type

struct subscribe_t {
    void operator ()(request& r, initializer_list<string_view>);
    // maybe other overloads
};

so push2 can just call into the passed object forwarding all the args. This way we can define each command (or family of commands) in a separate header.

@mzimbres
Copy link
Collaborator

It'd much better for usability having functions in request, actually. But then request is gonna end up having zounds of functions, and not all of them used frequently.

This is what I like most so far. I don't mind having lots of member function and overloads for each command. The complexity is not our invention but intrinsic to Redis. The only question is what to do when Valkey and Redis have a different definition for the same command.

@anarthal
Copy link
Collaborator Author

How about

req.push2(subscribe).with({"ch1", "ch2"});

The difference is that we avoid forwarding args in push2. This allows us to use things like initializer lists without incurring in problems.

I'm wary of adding too many member functions to request. We can easily end up with 1000 member functions if we end up adding support for all commands, each one having different overloads. This may not be the best for compile times.

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.

Pubsub state restoration

2 participants