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

full binary support #47

Closed
costin opened this issue Jan 11, 2011 · 11 comments
Closed

full binary support #47

costin opened this issue Jan 11, 2011 · 11 comments
Assignees
Labels

Comments

@costin
Copy link

costin commented Jan 11, 2011

Currently JRedis provides binary support for values but not for keys. Since Redis 2.0 uses binary-safe strings, such a feature will help greatly for storing data in binary format.

@alphazero
Copy link
Owner

Costin,

An api change.

Can you help convince me by giving me examples of what sort of keys you can not use right now?
And what sort of gymnastics (Romania :) are you willing to go through in terms of switching between semantic e.g. pure binary or classic String, interfaces to JRedis in your code?

Thank you for you feedback.

@costin
Copy link
Author

costin commented Apr 14, 2011

A string is a byte[] array with a certain encoding. So storing data that falls outside the encoding range (think dealing with i18n and multiple encodings inside the same app and thus db), thumbnails or crypto keys simply doesn't work unless doing all sort of unneeded tricks to fit the data into the String. Change the JVM encoding and you have to start over.

Redis is binary safe and libraries such as Jedis support this natively.
As for my usage, in Spring Data we give the choice to the user. There's no 'semantic' switch - Strings again are just byte[] with a range of chars (encoding). You can just as well save your objects as XML/Json or use Java Serialization just to name a few options.
We always use the binary API , it's the most generic one. In case of JRedis, we go through the gymnastics of encoding the binary data all the time (base64) which is no surprise, kills performance.

@alphazero
Copy link
Owner

Understood about the encoding but needed use-cases: crypto-keys. convinced me.

@ghost ghost assigned alphazero Apr 14, 2011
alphazero pushed a commit that referenced this issue Apr 15, 2011
Review changes in test suite to pin point the API methods that are no
longer backwards compatible.  These are a tiny subset of commands that
would return a key (not a value), such as RANDOMKEY.  Instead of
returning String, they now return byte[].  All other API changes are
backwards compatible so this should be of minimal pain to existing
applications.
@alphazero
Copy link
Owner

Redis 2.2.n client.
branch: master
commit: 956a874

@costin
Copy link
Author

costin commented Apr 15, 2011

That was fast! Thanks - by the way, are there any nightly snapshots for JRedis?

@alphazero
Copy link
Owner

no plans for nightly snapshots. you guys could help out by running your binary tests against the latest. I did a very trivial test in the hello redis example. The rest of the test suite is still using String keys so do let me know if you see any issues. Specially helpful would be hash tests.

@costin
Copy link
Author

costin commented Apr 19, 2011

Hi,

I've started testing the code and found the following issues:
a. List is still used as a return type on JRedis (by keys() and keys(K keys)).
b. mget uses Strings instead of byte[]
c. KeyValueSet uses String as a key
d. list/set/zset operations such as sdiff and sunion are still String based
e. hash operations (i.e. hset) use String for a field (instead of K)

@costin costin reopened this Apr 19, 2011
@alphazero
Copy link
Owner

Hi, thanks for catching that.

btw, I gather (please confirm) that at least on your end async ops are not supported?

@costin
Copy link
Author

costin commented Apr 19, 2011

There is work to add support for delayed results. Not sure whether this is what you meant by "async ops"...

alphazero pushed a commit that referenced this issue Apr 23, 2011
alphazero pushed a commit that referenced this issue Apr 23, 2011
per existing regression tests but this really needs to be banged on.
alphazero pushed a commit that referenced this issue Apr 23, 2011
Introduced new DefaulCode.toDataDictionary() helper method to convert
Map<byte[] ,byte[]> to Map<String, byte[]>.  (See affected tests for
usage examples.)

Tested.
alphazero pushed a commit that referenced this issue Apr 23, 2011
@alphazero
Copy link
Owner

Issue GH-47 (wip) -- sunion, sunionstore, sdiff and sdiffstore (future
commit de5a329

Issue GH-47 (wip) -- sinterstore (future was written) -- TESTED
commit 0a0f70c

Issue GH-47 (wip) -- sinter() (future was written) TESTED.
commit 079367c

Issue GH-47 (wip) -- rest of the hash methods.
commit 1e8e3a3

Issue GH-47 (wip) -- hset binary support. (Future was written.)
commit d1a74c7

Issue GH-47 (wip) -- KeyValueSet and co. (that was 'fun' ..) -- TESTED
commit 8c9f4a4

Issue GH-47 (wip) -- keys() and mget binary support. (The future was
commit cf65061

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants