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 blpop and adjusted to signed 64 bit numbers #527

Merged
merged 3 commits into from Feb 18, 2014

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Feb 14, 2014

Because we're finalizing the Redis API, I thought it would be important to make some better decisions.

First, it's very unsafe for the API to use size_t b/c on i386 platforms forcing 32 bit integers could result in data loss considering Redis sends a string of a 64 bit integers always.

I added bblpop as a blocking list pop and a non-blocking list blpop as a task with concurrency. An issue I pointed out earlier was that redis was not precise on its timeout. The suggestion was to close the connection after 10 seconds, but using a task the best workaround would be to run :

auto value = redis.blpop!(string[])(10.seconds, "list1", "list2").receiveTimeout!string(10.seconds);
redis.lpush("list1", "1"); // satisfies the receive loop in blpop

This way, the connection will be recycled and the timeout ends nearer to 10s rather than 10.66s+.

I'd also like to suggest changing the template parameters, ie. lindex(T : E[], E) should be lindex(T) b/c there would be more logic requesting the response type rather than specifying the underlying range.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 15, 2014

Member

Good point with the size, but should it be long or ulong?

I don't like the bblpop. "blpop" means blocking pop, so it should, well, block. The asynchronous version also doesn't really provide enough value to warrant an extra method (btw. it sends to the wrong task). Generally the goal should be to make developers aware of runTask as a standard means to achieve such effects instead of providing wrappers for one-liners. listen is an exception here, because it will supposedly be the expected behavior in most cases.

The lpush method to force a timeout I guess can't work in general, because it will influence other clients and is prone to race conditions.

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string? I think everything else (serialization/deserialization of other types) should better be left to the user in that case. But I don't really have an overview of how Redis is organized there.


What would possibly make sense for blpop though, would be to implement a precise timeout like this:

T blpopPrecise(Duration timeout, string[] keys...)
{
    if (timeout <= 0.seconds) return request!T("blpop", keys, 0);
    auto t = Task.getThis(); // stored in heap closure
    bool receiving = true; // stored in heap closure
    runTask({
        // round up the duration to avoid premature timeouts
        auto ret = request!T("blpop", keys, (timeout + 999.msecs).total!"seconds");
        if (receiving) t.send(ret);
        else lpush(ret); // hmm... this is wrong, is there anything to push to the end of a list instead?
    });
    T ret; // return null in case of a timeout
    if (receiveTimeout(timeout, (T val) { ret = val; });
    receiving = false;
    return ret;
}

But due to the additional cost caused be the heap closure, I'd suggest to make this an additional method instead of the only one.

Member

s-ludwig commented Feb 15, 2014

Good point with the size, but should it be long or ulong?

I don't like the bblpop. "blpop" means blocking pop, so it should, well, block. The asynchronous version also doesn't really provide enough value to warrant an extra method (btw. it sends to the wrong task). Generally the goal should be to make developers aware of runTask as a standard means to achieve such effects instead of providing wrappers for one-liners. listen is an exception here, because it will supposedly be the expected behavior in most cases.

The lpush method to force a timeout I guess can't work in general, because it will influence other clients and is prone to race conditions.

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string? I think everything else (serialization/deserialization of other types) should better be left to the user in that case. But I don't really have an overview of how Redis is organized there.


What would possibly make sense for blpop though, would be to implement a precise timeout like this:

T blpopPrecise(Duration timeout, string[] keys...)
{
    if (timeout <= 0.seconds) return request!T("blpop", keys, 0);
    auto t = Task.getThis(); // stored in heap closure
    bool receiving = true; // stored in heap closure
    runTask({
        // round up the duration to avoid premature timeouts
        auto ret = request!T("blpop", keys, (timeout + 999.msecs).total!"seconds");
        if (receiving) t.send(ret);
        else lpush(ret); // hmm... this is wrong, is there anything to push to the end of a list instead?
    });
    T ret; // return null in case of a timeout
    if (receiveTimeout(timeout, (T val) { ret = val; });
    receiving = false;
    return ret;
}

But due to the additional cost caused be the heap closure, I'd suggest to make this an additional method instead of the only one.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 16, 2014

Contributor

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string?

Ok so API calls are intentionally restricted to a response range & disallow individual values.

hmm... this is wrong, is there anything to push to the end of a list instead?

That would be rpush. The "shared" bool idea is ideal

should it be long or ulong?

"the returned integer is guaranteed to be in the range of a signed 64 bit integer."

instead of providing wrappers for one-liners.

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the Task type, as is on both ends.. I'm still thinking about that).

Contributor

etcimon commented Feb 16, 2014

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string?

Ok so API calls are intentionally restricted to a response range & disallow individual values.

hmm... this is wrong, is there anything to push to the end of a list instead?

That would be rpush. The "shared" bool idea is ideal

should it be long or ulong?

"the returned integer is guaranteed to be in the range of a signed 64 bit integer."

instead of providing wrappers for one-liners.

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the Task type, as is on both ends.. I'm still thinking about that).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 16, 2014

Member

That would be rpush. The "shared" bool idea is ideal

Oh okay... and I thought "lpush" meant "list push" all the time :)

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the task type on both ends.. I'm still thinking about that).

But especially if there is a planned concept that will supersede such an API it makes no sense to introduce it now. Only APIs that are supposed to stay forever should be added now. Especially since the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

Member

s-ludwig commented Feb 16, 2014

That would be rpush. The "shared" bool idea is ideal

Oh okay... and I thought "lpush" meant "list push" all the time :)

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the task type on both ends.. I'm still thinking about that).

But especially if there is a planned concept that will supersede such an API it makes no sense to introduce it now. Only APIs that are supposed to stay forever should be added now. Especially since the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 17, 2014

Contributor

Oh okay... and I thought "lpush" meant "list push" all the time :)

lol I understand that feeling too well

the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

Yes indeed.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

Contributor

etcimon commented Feb 17, 2014

Oh okay... and I thought "lpush" meant "list push" all the time :)

lol I understand that feeling too well

the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

Yes indeed.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 18, 2014

Member

Thanks, as far as I can tell everything looks good.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

Agreed, this is too important to not fix and there is no obvious deprecation path in this case.

Member

s-ludwig commented Feb 18, 2014

Thanks, as far as I can tell everything looks good.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

Agreed, this is too important to not fix and there is no obvious deprecation path in this case.

s-ludwig added a commit that referenced this pull request Feb 18, 2014

Merge pull request #527 from globecsys/master
Added blpop and adjusted to signed 64 bit numbers

@s-ludwig s-ludwig merged commit dfd0f7d into vibe-d:master Feb 18, 2014

1 check passed

default The Travis CI build passed
Details
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 20, 2014

Contributor

I was exploring const-correctness and thinking, shouldn't the RedisClient method return types all use immutable storage, and the parameters all have the in specifier?

Contributor

etcimon commented Feb 20, 2014

I was exploring const-correctness and thinking, shouldn't the RedisClient method return types all use immutable storage, and the parameters all have the in specifier?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 20, 2014

Member

As far as I can see, most parameters/return values are POD types, so they wouldn't benefit from const/immutable. Then there are some string or string[]... parameters, which are already immutable because string is immutable. And then there are the methods taking generic types as parameters and those should also already accept const() values. The only different case is for RedisReply return values, but this is kind of a special case because the reply has to be modified while reading out the response. Any other cases?

Member

s-ludwig commented Feb 20, 2014

As far as I can see, most parameters/return values are POD types, so they wouldn't benefit from const/immutable. Then there are some string or string[]... parameters, which are already immutable because string is immutable. And then there are the methods taking generic types as parameters and those should also already accept const() values. The only different case is for RedisReply return values, but this is kind of a special case because the reply has to be modified while reading out the response. Any other cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment