dartist / redis_client

A high-performance async/non-blocking Redis client for Dart
BSD 2-Clause "Simplified" License
100 stars 29 forks source link

v0.2.0 - Refactor to latest dart version #10

Closed enyo closed 10 years ago

enyo commented 11 years ago

This branch is not ready yet! Do not merge.

Refactore RedisClient

DONE

I actually removed RawRedisCommands as well.

If someone wants to talk to the Socket directly, RedisConnection.send() and RedisConnection.rawSend() can be used. I would dump the RawRedisClient in favor of a single unified RedisClient that has a RawRedisCommands object, which it invokes on the few functions that differ.

If somebody actually needs to access those raw functions, it's accessible through: myRedisClient.raw.setget();

This way there's...

  1. ...only one place that defines all the available methods (which is perfect for generating a good documentation API which I'll do as a next step),
  2. ...no need for the interfaces anymore,
  3. ...no copy past wraps of the "Native" Client,
  4. ...a reduced risk of errors since there's much less repetitive code.

    Drop the callback functions

DONE

I'd remove the callback functions, and go for futures instead.

The syntax would be (much like most of dart's IO code):

RedisClient.connect(connectionString).then((RedisClient client) {
  // Do stuff
});

The same goes for the onClosed callback:

client.done.then(() => /* do stuff */);

This is pretty much the same API as dart's own Socket

Create a RedisProtocolTransformer

DONE

The redis replies should be transformed by a RedisProtocolTransformer so it's possible to just do this:

redisSocket.transform(new RedisProtocolTransformer()).listen((RedisReply reply) => );

Small stuff

enyo commented 11 years ago

@financeCoding @mythz I hope it's OK for you that I created the milestone and the issues. Don't want to be too intrusive, but it helps to keep my workflow.

If you're annoyed by it, I can use my fork as an issue / milestone list.

adam-singer commented 11 years ago

@enyo its fine, not intrusive :+1:

mythz commented 11 years ago

Yep no probs here either - totally fine with it.

enyo commented 11 years ago

I have a few questions concerning the source.

First the BytesEncoder. Isn't this more of a serializer than an encoder?

And I don't quite get the stringify function: https://github.com/Dartist/RedisClient/blob/master/lib/src/RedisClient.dart#L364

I get that you have a special format for booleans, and Dates, but what if the object that gets inserted is a Map containing Dates? Why not just use JSON.stringify and insert that?

mythz commented 11 years ago

Yep I usually say "Serializer" myself, but there must've been something in the Dart SDK that made me call it an "Encoder" - feel free to rename it.

It looks like Date's in a Map will render differently which is not ideal, looks like I might need to investigate this further - the original goal was to maintain compatibility with the JSON stored by my C# Redis Client.

enyo commented 11 years ago

@mythz Ok, that explains it. Is this compatibility with C# important? Because simply calling JSON.stringify would solve the whole problem and be a bit more stable, wouldn't it?

I don't quite see the use case where a C# app, and a dart app need to share the same redis information.

mythz commented 11 years ago

Redis can be used like a conduit to efficiently communicate and share data between processes in different langs, either via normal data storage or in real-time via pub/sub.

E.g. eventually I plan to create Dart bindings for Redis MQ

mythz commented 11 years ago

But that's just some bg context - lets not let that prevent changes as I can just add support for Dart Dates in my C# Json Serializer

enyo commented 11 years ago

@mythz that makes sense (using redis as communication for data). Using JSON just seems as the sanest thing to do since it will be the easiest for other applications to parse.

Out of curiosity: why did you choose to go for your own format of Date and booleans in your C# implementation?

mythz commented 11 years ago

Wasn't my own format, I made it so it was backwards compatible with WCF's JSON Date Format which includes some good reasons with why the chose it - sending epoch (UTC) ticks on the wire does make it less problematic to parse/convert in different langs.

adam-singer commented 11 years ago

+1 on Redis MQ

enyo commented 11 years ago

Hi.. just wanted to let you know that I underestimated the work on the RedisConnection a bit and it's taking longer than expected :) But I haven't given up on it yet.

I've decided to implement a RedisProtocolTransformer to properly convert the Stream to RedisReplys (I've added the information in the pull request description).

mythz commented 11 years ago

@enyo yep, always thought it was going to be a non-trivial effort :)

enyo commented 11 years ago

Especially since a lot has changed in dart since you wrote the library. So sometimes it's a bit hard to actually get the meaning of some functions because the documentation on the original dart functionality is gone.

All in all I have to say that the project is quite fun and it's great developing from your code base. I also haven't found any bugs so far except for a tiny one: https://github.com/Dartist/RedisClient/blob/master/lib/src/RedisConnection.dart#L570 I think that line should actually be:

if (stream.available() < (count + 2)) return;

since the redis spec says that the actual response is followed by the two bytes for the final CRLF.

adam-singer commented 10 years ago

nice! Going to make a pub publish soon?

MaxHorstmann commented 10 years ago

@financeCoding done

adam-singer commented 10 years ago

:+1: