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] Flexibility to set socket_timeout for each request to redis #307

Open
eriktuantran opened this issue Dec 23, 2021 · 11 comments
Open

Comments

@eriktuantran
Copy link

Hi @sewenew
I'm using RedisCluster and running into the situation that different socket_timeout values for each request are needed:

  • When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.
  • When triggering a redis command that takes long time to finished, I need another socket_timeout value that long enough to wait for the reply.

The socket_timeout value will be decided by application. However, it looks like that we can not adjust the socket_timeout for each redis request for now.
It would be great that we can support this feature. How do you think?

@sewenew
Copy link
Owner

sewenew commented Dec 27, 2021

When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.

First of all, how can you tell if a redis instance is going down? If it's already down, socket_timeout won't help. Since it will fail to reconnect to Redis, and won't send any command to it.

When triggering a redis command that takes long time to finished, I need another socket_timeout value that long enough to wait for the reply.

First of all, it's not a good idea to run a long running command with Redis, since it's single-threaded, and the long running command will block Redis. If you do need to do that, I think you'd better use a dedicated Redis object to do the job. Because Redis operation is blocking, if a connection is waiting for a command for a long time, the connection will not be returned to the underlying connection pool, and other threads could not reuse this connection either.

It would be great that we can support this feature. How do you think?

If this feature is added, the API might change a lot. I'm not sure if this is a good idea. So I'll keep this issue open, but hold this request for now. If there're many others need this feature, I'll reconsider it.

Anyway, thanks for your proposal!

Regards

Regards

@wingunder
Copy link
Contributor

Hi @sewenew,
I actually have a use-case for this request:

I am handling data from sensors that deliver their data at pre-defined intervals. Eg. sensor A, @ a 3 second interval, sensor B @ a 15 second interval, etc. My orchestration of the handling of the data, is data-triggered. This means I need to know if a sensor stopped delivering data. If so, I need to trigger the data handling in any case, marking the data as invalid.

At the moment, I have a C++ process with a thread per sensor. In order not to use a Redis instance per thread, I use a reduced socket_timeout and only handle the data under special conditions, when a timeout occurs.
Here is a piece of code, roughly explaining it with a socket_timeout of 4 seconds, sensor A @ 3 sec and sensor B @ 15 sec. The code can be scaled to many sensors/inputs, using only one Redis instance.

sw::redis::ConnectionOptions connectionOptions;
connectionOptions.socket_timeout = std::chrono::seconds(4);
sw::redis::Redis redis(connectionOptions);

std::thread a(processValue(std::ref(redis), "A", std::chrono::seconds(3), std::ref(connectionOptions.socket_timeout)));
std::thread b(processValue(std::ref(redis), "B", std::chrono::seconds(15), std::ref(connectionOptions.socket_timeout)));

void processValue(sw::redis::Redis& redis,
                  const std::string& key,
                  const std::chrono::seconds& interval,
                  const std::chrono::seconds& timeout)
{
    auto sub = redisInstance.subscriber();
    sub.on_message([](std::string key, std::string val) {doCalc(key, val, true);});

    sub.subscribe(key);
    sub.consume();

    std::chrono::seconds subTimeWithoutData(0);
    while (running) {
        try {
                sub.consume();
                subTimeWithoutData = std::chrono::seconds(0);
        }
        catch (const sw::redis::TimeoutError &e) {
            subTimeWithoutData = subTimeWithoutData + timeout;
            if (subTimeWithoutData > interval) {
                // Reduce the subTimeWithoutData accordingly.
                subTimeWithoutData = subTimeWithoutData - interval;
                // Do the calc with invalid data.
                doCalc(key, dummyVal, false);
            }
        }
    }
    sub.unsubscribe(key);
    sub.consume();
}

Having a (p)subscribe or a consume method that takes a timeout value, would simplify the implementation of the above use-case enormously.

BTW, I cannot think of any non-pub/sub kind of commands that would benefit from custom timeouts, as in the real world all non-pub/sub commands should be super fast with their response, otherwise something is wrong in any case.

Regards

@wingunder
Copy link
Contributor

Hi @sewenew and @quoctuanuit,

@quoctuanuit wrote:

When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.

Like @sewenew said, you might have a design problem.
It depends on the use-case, but using streams and acking each stream item, once successfully handled, is a way to prevent loosing stuff in some cases.
On the other hand, if you're using RedisCluster, your cluster should failover without problem. If your cluster really goes down, you'll either need to add hardware to make it more robust, or simply cope with the disaster.

@sewenew wrote:

First of all, how can you tell if a redis instance is going down?

Dead Redis nodes can be determined independently, by subscribing to specific keys of a Sentinel server.
Sentinel can also be used in combination with RedisCluster.

Regards

@sewenew
Copy link
Owner

sewenew commented Dec 29, 2021

@wingunder

I actually have a use-case for this request:
....
In order not to use a Redis instance per thread

In this case, you CAN have a Redis instance per thread. Because Redis::subscriber always create a Subscriber with a new connection, and the Redis object creates connection lazily. So even if you have a Redis object per thread, there will be only one connection to Redis server per thread. In a word, in your case, a Redis object per thread works the same as a global Redis object.

std::thread a(processValue("A", std::chrono::seconds(3)));
std::thread b(processValue("B", std::chrono::seconds(15)));

void processValue(const std::string& key,
                  const std::chrono::seconds& timeout)
{
    sw::redis::ConnectionOptions connectionOptions;
    connectionOptions.socket_timeout = timeout;
    sw::redis::Redis redis(connectionOptions);           // <------ since no command is sent with Redis object, no connection will be created.
    auto sub = redis.subscriber();   // <----- always create a new connection.
    sub.on_message([](std::string key, std::string val) {doCalc(key, val, true);});

    sub.subscribe(key);
    sub.consume();

    while (running) {
        try {
                sub.consume();
        }
        catch (const sw::redis::TimeoutError &e) {
                doCalc(key, dummyVal, false);
            }
        }
    }
    sub.unsubscribe(key);
    sub.consume();
}

Regards

@sewenew
Copy link
Owner

sewenew commented Dec 29, 2021

@wingunder

Dead Redis nodes can be determined independently, by subscribing to specific keys of a Sentinel server.

Yes, you can be notified by Sentinel, but only when the node is already down. However, if I understood correctly, I think @quoctuanuit want to modify the timeout before the node is down, i.e. the node is still alive, but is going to be down.

Correct me if I misunderstand his question, since I'm not a native speaker :)

Regards

@wingunder
Copy link
Contributor

Hi @sewenew,

In this case, you CAN have a Redis instance per thread.

Thanks for your explanation and your example.
The missing clue for me was:

    sw::redis::Redis redis(connectionOptions); // <------ since no command is sent with Redis object, no connection will be created.

Would it be possible to mention this example in the documentation, or maybe even enhance the documentation with an example like this?

BTW, this information could also have been helpful for #240.

Regards

@wingunder
Copy link
Contributor

Hi @sewenew,

However, if I understood correctly, I think @quoctuanuit want to modify the timeout before the node is down, i.e. the node is still alive, but is going to be down.

Correct me if I misunderstand his question, since I'm not a native speaker :)

I read @quoctuanuit's explanation again and I see your point. One could read it the way you explained. So, in this case something that will bring a node down, will tell his application to reduce the timeout, so that the app can know almost immediately when things are not working any more.

I however think he basically wants a very short timeout so that he can know that something is wrong, as soon as a node is down or not responding any more. I don't know of a way to automatically detect if a node is on its way down (not down, but will be down soon). Maybe @quoctuanuit should try to explain this to us, if it is still relevant.

Anyway, I think that his real issue was to be able to set the socket_timeout to some value, before any redis call:

However, it looks like that we can not adjust the socket_timeout for each redis request for now.

This is indeed not possible at the moment. However, after reading the example that you posted a few moments ago, I really see no use-case for being able to adjust the socket_timeout before every redis call.

Regards

@sewenew
Copy link
Owner

sewenew commented Dec 30, 2021

Hi @wingunder

Thanks for your suggestion! I've updated the doc, and you can take a look: commit .

Regards

@wingunder
Copy link
Contributor

Hi @sewenew,

Thanks for your suggestion! I've updated the doc, and you can take a look: commit .

You're welcome. Thanks for adding this to the documentation.

Regards

@eriktuantran
Copy link
Author

Hi @sewenew @wingunder !
Sorry for the late response!
You are right @sewenew that I want to modify the timeout before the node is down, then the error will be returned as fast as possible.
In my situation, the redis client is running on a host machine, and the redis server is running in few pluggable devices.
The term "node is down" means that the pluggable devices is unplugged.
FYI, redis-server can be extended by redis-modules where we can add new redis commands other than the regular one (GET/SET...). The new redis command could take long time to finished (e.g it need to send the data to high latency hardware)
(Here is more info about redis-modules if you want: https://redis.io/topics/modules-intro)

One more benefit of setting socket_timeout per request is that application can control what command should be blocked until server replied, or should we have some timeout for it.

"since it's single-threaded, and the long running command will block Redis."

Well, I believe that it's not single-threaded, e.g every blocking module command is handled by it's own thread in redis.

@sewenew
Copy link
Owner

sewenew commented Jan 9, 2022

One more benefit of setting socket_timeout per request is that application can control what command should be blocked until server replied, or should we have some timeout for it.

In this case, so far, I'd suggest that you create different Redis or RedisCluster objects for different tasks. So that you can set different socket_timeout for these Redis objects.

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

3 participants