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

Stream feature polish #811

Merged
merged 3 commits into from
Dec 13, 2018
Merged

Stream feature polish #811

merged 3 commits into from
Dec 13, 2018

Conversation

byroot
Copy link
Collaborator

@byroot byroot commented Dec 10, 2018

While playing with #799 locally, I realized it uses some modern Ruby feature like Array#concat varargs signature.

This PR updates the builds matrix so that it spots more of this kind of issues.

I'll wait for the results and fix what comes up.

@byroot
Copy link
Collaborator Author

byroot commented Dec 10, 2018

cc @supercaracal just so you know.

- 2.3.3
- 2.4.1
- 2.5.0
- 2.3.8
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the rubies to recent point releases, and dropped 2.2.x since it's now EOL. The matrix is already huge so we can't realistically test EOL versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to update required_ruby_version?

- DRIVER=ruby REDIS_BRANCH=4.0
- DRIVER=ruby REDIS_BRANCH=5.0
- DRIVER=hiredis REDIS_BRANCH=5.0
- DRIVER=synchrony REDIS_BRANCH=5.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea here is to test all the versions of the server with the ruby driver, but only the most recent one with the alternative drivers. I believe it's a good compromise between coverage and matrix size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, do we need JRuby x Redis 5.0 pattern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the only one we should test actually IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@byroot
Copy link
Collaborator Author

byroot commented Dec 10, 2018

@supercaracal another thing I spotted and I'd like your opinion on. XREAD returns a hash of a hash:

>> pp r.xread('mystream', '0-0', block: 50_000)
{"mystream"=>
  {"1544447512733-0"=>{"f1"=>"v1", "f2"=>"v2"},
   "1544447564923-0"=>{"f1"=>"v2", "f2"=>"v5"}}}

I wonder if it actually make sense, and if arrays wouldn't be more appropriate:

{"mystream"=> [
  ["1544447512733-0", {"f1"=>"v1", "f2"=>"v2"}],
  ["1544447564923-0",{"f1"=>"v2", "f2"=>"v5"}]
]}

Granted, hashes are ordered in Ruby, but I'm not sure addressing the result by id actually make sense. What do you think?

@supercaracal
Copy link
Contributor

@byroot I think so too. I'm sorry. Hash is excessive abstraction. Array is more intuitive than Hash.

@byroot
Copy link
Collaborator Author

byroot commented Dec 10, 2018

No need to be sorry !

@byroot
Copy link
Collaborator Author

byroot commented Dec 10, 2018

@supercaracal I pushed a change so that we use start and end as params to be consistent with the server docs. Mind taking a look when you have time?

@supercaracal
Copy link
Contributor

@byroot 👍 I agree it. It is match Redis' API one-to-one. There is no learning cost for user.

@byroot
Copy link
Collaborator Author

byroot commented Dec 11, 2018

Actually I'm now wondering if we should use named arguments at all for start and end.

def xrange(key, start='-', _end='+', count: nil)
def xrevrange(key, _end='+', start='-', count: nil)

That's probably as easy to use if not more ^

@supercaracal
Copy link
Contributor

supercaracal commented Dec 11, 2018

@supercaracal
Copy link
Contributor

supercaracal commented Dec 11, 2018

Perhaps, we should use named optional arguments for the count option too. e.g. #spop #srandmember

@byroot
Copy link
Collaborator Author

byroot commented Dec 11, 2018

I think named arg for count make sense.

I feel like a common use for xrange is "give me the last X events", so xrange(key, count: X)

Another likely common use case is "give me events since X, but no more than 50", so xrange(key, last_id, count: 50).

@supercaracal
Copy link
Contributor

Certainly. I understand.

@supercaracal
Copy link
Contributor

supercaracal commented Dec 11, 2018

I'd say that the use cases of specifying XRANGE's end or XREVRANGE's start are rare.

I think that the following argument pattern is easy-to-use and consistent.

  • start or end options are named optional arguments
  • count option is keyword argument

@byroot
Copy link
Collaborator Author

byroot commented Dec 11, 2018

I'm not sure I understand the distinction you make between keyword and named arguments. Care to explain ?

@supercaracal
Copy link
Contributor

supercaracal commented Dec 11, 2018

I'm sorry. I made a mistake. I chose wrong English word. s/named/optional/

@supercaracal
Copy link
Contributor

supercaracal commented Dec 12, 2018

I apologize for the above my confused comments. I mixed up the named arguments and the optional arguments. The named arguments are same as the keyword arguments. I'm sorry.

If we should adjust to Redis command interface:

# xread(key, start, _end, count = nil)
r.xread('s1', '-', '+')
r.xread('s1', '-', '+', 10)
r.xread('s1', '0-9', '+', 10)
  • Pros
    • simple and intuitive
    • consistent for other methods
  • Cons
    • we have to specify start and end arguments every time

If we have priority to easy-to-use interface in Ruby:

# xread(key, start = '-', _end = '+', count: nil)
r.xread('s1')
r.xread('s1', count: 3)
r.xread('s1', '0-1')
r.xread('s1', '0-1', '0-9')
r.xread('s1', '0-1', count: 3)
  • Pros
    • easy-to-use
  • Cons
    • need a few learning cost for the abstraction

I'd say that the latter looks good.

@byroot byroot merged commit 7ff90f1 into master Dec 13, 2018
@byroot byroot deleted the stream-improvements branch December 13, 2018 13:11
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.

2 participants