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/streams #78

Closed
wants to merge 2 commits into from
Closed

Feature/streams #78

wants to merge 2 commits into from

Conversation

aemadrid
Copy link

Adds support for all stream related commands in value mode. Mostly porting the code from the ruby driver.

Note: Still need to investigate how to support futures for pipelining.

This was referenced Jan 29, 2019
@larskluge
Copy link

larskluge commented Feb 11, 2019

I tried the very basics and it's looking good so far. All as expected/known from its Ruby counterpart.

And built a tiny first project based on it: https://github.com/larskluge/rsone

Will report if I find anything down the road.

Thanks for your work @aemadrid !

@aemadrid
Copy link
Author

@larskluge thx! I've looked at implementing 1hashify1/etc for pipelines/futures but I cannot seem to get it to work. Not sure how you feel about having streams not working there.

@stefanwille
Copy link
Owner

@aemadrid thanks for the PR. This looks like good start.

If you can get away with using the builtin commands primitives such as string_command(), there is a good chance that your implementation works with pipelines/futures too.

I see the spec fails in CI. Is it green for you in local development?

@aemadrid
Copy link
Author

@stefanwille it does pass locally. I think the problem is that travis doesn't have the right redis version (>= 5). BTW I figured out a solution to the pipeline issue but it will be a much bigger PR. My idea is to command(args, unwrapper) where unwrapper is a strategy like :int_or_nil, :string_array, etc. so that could be applied at the right time. Very similar to what the ruby driver does. That should work for pipelines, transactions, etc. and also work for all hash, stream, etc. responses.

@Xosmond
Copy link

Xosmond commented Jun 26, 2019

@aemadrid @stefanwille Is there any news on this PR?

@stefanwille
Copy link
Owner

No

if result.is_a? Array(Redis::RedisValue)
result.each do |arr|
if arr.is_a? Array(RedisValue)
ary << {
Copy link
Contributor

@dscottboggs dscottboggs Sep 22, 2019

Choose a reason for hiding this comment

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

This should certainly be a struct

struct StreamPendingDetails
  property entry_id : String
  property consumer : String
  property elapsed : String
  property count : String
  def initialize(result : Array(RedisValue))
    @entry_id = result[0].to_s
    @consumer = result[1].to_s
    @elapsed = result[2].to_s
    @count = result[3].to_s
  end
  def self.new_collection(result : Array(RedisValue)) : Array(StreamPendingDetails)
    Array(StreamPendingDetails).new.tap do |ary|
      result.each do |result_ary|
        ary << StreamPendingDetails.new result_ary
      end
    end
  end
end

There's no sense in using a hash where all potential keys are known.

@anykeyh
Copy link

anykeyh commented Aug 9, 2022

Hello,

I was planning to work precisely on this feature for a project of mine. Very happy that someone already started it <3.

However, what is currently blocking this from being merged to master? Except for the travis Redis version, is there anything needed to be implemented?

I would be happy to help.

@kostya
Copy link
Collaborator

kostya commented Aug 9, 2022

I think it mergable, but need update to master (btw method hashify was already added). Then if specs passing, also check that this feature works in real.

@aemadrid aemadrid closed this Feb 16, 2023
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.

7 participants