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.

Sign up for GitHub

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

Jump to bottom

Add Streams support #799

Merged
merged 1 commit into from
Dec 10, 2018
Merged

Add Streams support #799

merged 1 commit into from
Dec 10, 2018

Conversation

supercaracal
Copy link
Contributor

I'd like to use Redis Streams.

@supercaracal
Copy link
Contributor Author

supercaracal commented Nov 22, 2018 •
edited
Loading

ref: redis/redis-py#1040
ref: redis/redis#5211
ref: #810

byroot
byroot reviewed Nov 22, 2018
View reviewed changes
Copy link
Collaborator

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Just some minor nitpicky comments.

The code looks good, but I need to take some time to think about the API.

lib/redis.rb Outdated
HashifyStreamEntries =
lambda { |reply|
reply.map do |entry_id, values|
[entry_id, Hash[values.each_slice(2).to_a]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

values.each_slice(2).to_h is simpler.

lib/redis.rb Outdated
synchronize { |client| client.call([:xlen, key]) }
end

# Fetches entries from one or multiple streams. You're able to use blocking if needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're able to use blocking if needed. I think this needs to be phrased differently.

Maybe Optionally blocking ?

lib/redis.rb Outdated
synchronize { |client| client.call(args, &HashifyStreamEntries) }
end

# Fetches entries of the stream ordered by descending.
Copy link
Collaborator

Choose a reason for hiding this comment

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

ordered by descending. -> in descending order.

@supercaracal supercaracal force-pushed the add-streams-support branch 4 times, most recently from 2f9bdff to 5a9305b Compare November 23, 2018 09:07
@supercaracal
Copy link
Contributor Author

I have fixed the following additional things.

  • Fix several wrong or not enough YARD comments (e.g. @return tag)
  • Add #_xread private method for DRY between #xread and #xreadgroup

@supercaracal
Copy link
Contributor Author

supercaracal commented Nov 23, 2018 •
edited
Loading

I have fixed as the following.

  • Refactor optional parameters of methods
  • Change option naming concerns #xtrim and #xadd methods into a more meaningful word
    • s/modifier/approximate/

@supercaracal supercaracal force-pushed the add-streams-support branch 4 times, most recently from 53105c4 to c17db7f Compare November 28, 2018 14:12
@benlangfeld benlangfeld mentioned this pull request Nov 30, 2018
Closed
8 tasks
byroot
byroot reviewed Dec 10, 2018
View reviewed changes
test/helper.rb
@@ -159,6 +159,10 @@ def target_version(target)
end
end

def omit_version(min_ver)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't used anywhere. Can you remove it from the PR please?

Copy link
Contributor Author

@supercaracal supercaracal Dec 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The above helper method is used by the following linter.
https://github.com/redis/redis-rb/pull/799/files#diff-a14ac8847c163e36437c11a9aa3c7643R8-R11

test/lint/streams.rb
Lint::Streams#setup

Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird, that's a broken link for me and ctrl + F omit_version doesn't match anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Argh, nvm, GitHub chose to hide that part of the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry.

def setup
  super
  omit_version(MIN_REDIS_VERSION)
end

@byroot
Copy link
Collaborator

byroot commented Dec 10, 2018

I think this looks very good.

It just need a rebase and some minor ajustement.

I'd like to merge it and play with it a bit, and eventually release a 4.1.0 with this & the cluster support.

@byroot
Copy link
Collaborator

byroot commented Dec 10, 2018

Can you rebase ? I'll merge afterward.

@supercaracal
Copy link
Contributor Author

supercaracal commented Dec 10, 2018 •
edited
Loading

I have rebased. I appreciate for your review.

@byroot byroot merged commit e036bcd into redis:master Dec 10, 2018
@supercaracal supercaracal deleted the add-streams-support branch December 10, 2018 12:09
@byroot byroot mentioned this pull request Dec 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byroot byroot byroot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@supercaracal @byroot