BinaryJedisCommands, ScriptingCommands, BinaryScriptingCommands

[phufool]

This pull request has the ScriptingCommands changes I had for the other pull request. I also included the implementations I made for BinaryScriptingCommands and BinaryJedisCommands for cluster. I also moved the BasicCommands from JedisCluster to BinaryJedisCluster.

An issue I found is the binary scripting command eval(final byte[] script, final byte[] keyCount, final byte[]... params). It returns the exception "Lua redis() command arguments must be strings or integers", even though the keyCount is a valid byte array.

[HeartSaVioR]

@phufool Hello!
At first, thanks for your (with @venkates) amazing work! I'm very appreciated it!

Btw, I've requested for pull about "removing BasicCommand from JedisCluster", #617 .
It was both agreed with @venkates since commands in BasicCommand are useless with Cluster mode, we cannot arrange which node takes request and doing some work.

Please review #617 , and when you can agree, please remove BasicCommand from BinaryJedisCluster.

Thanks again!

[HeartSaVioR]

@phufool It's strange that Jedis sends all eval parameters to byte array, so when you really make it right it should work well.
It seems that you missed something or Jedis has a bug.
Can you please post test case or code block you were failing?

I'm adding some lines to current unit test for simply run with binary, and it seems to no problem.

@Test
    public void scriptEvalReturnNullValues() {
    String script = "return {KEYS[1],KEYS[2],ARGV[1],ARGV[2]}";
    List<String> results = (List<String>) jedis.eval(script, 2, "key1", "key2", "1", "2");
    assertEquals("key1", results.get(0));
    assertEquals("key2", results.get(1));
    assertEquals("1", results.get(2));
    assertEquals("2", results.get(3));

    byte[] bScript = SafeEncoder.encode(script);
    List<byte[]> bResults = (List<byte[]>) jedis.eval(bScript, Protocol.toByteArray(2), 
        SafeEncoder.encode("key1"), SafeEncoder.encode("key2"), 
        SafeEncoder.encode("1"), SafeEncoder.encode("2"));
    assertArrayEquals(SafeEncoder.encode("key1"), bResults.get(0));
    assertArrayEquals(SafeEncoder.encode("key2"), bResults.get(1));
    assertArrayEquals(SafeEncoder.encode("1"), bResults.get(2));
    assertArrayEquals(SafeEncoder.encode("2"), bResults.get(3));
    }

[HeartSaVioR]

@phufool Maybe I found it.

Below is Protocol class.

    public static final byte[] toByteArray(final int value) {
    return SafeEncoder.encode(String.valueOf(value));
    }

You can find that we would be fine with "byte array made by string version of integer".
It means we could having problem if we directly convert primitive to byte array, and using this to parameter.

For example, below is BinaryJedis class.

public Object eval(byte[] script, int keyCount, byte[]... params) {
    client.setTimeoutInfinite();
    client.eval(script, SafeEncoder.encode(Integer.toString(keyCount)),
        params);
    return client.getOne();
    }

You can see that keyCount is not directly converted to byte array.
It's converted to String, and converted to byte array.

I think it leads confusing, so "commands with having byte array parameter for primitive type" could be discussed to deprecated, and finally removed.
@xetorthio @marcosnils What do you think about?

[HeartSaVioR]

According to http://redis.io/commands/blpop, arg is the key.
So you should call runBinary with arg.
Sorry for confused naming. I think arg should be changed to key.

[phufool]

I'll fix this so it passes arg

[HeartSaVioR]

@phufool Good job with fixing these! Thanks!
Btw, it seems that there is a compilation errors from your recent patch.
Could you fix it and make sure "make test" is success from your local PC and push to remote?

[phufool]

Hey HeartSaVioR,
I've removed a test case that used the ping method from basic commands. Is it ok now?

[HeartSaVioR]

@phufool Yes, it's working.
Thanks for quick fix!

[marcosnils]

@HeartSaVioR @phufool seems like this PR will definitely break backward compatibility. Do you agree to schedule it for 3.0?

[HeartSaVioR]

@marcosnils Surely yes.
Actually this PR is for cluster-revised branch.
We can grow cluster functionalities by branch and finally merge with 3.0.0.
So it can be merged right now. :)

[phufool]

@marcosnils Yes that sounds good!

[HeartSaVioR]

@phufool @marcosnils
Since it's connected to cluster-revised branch, so I merged this PR and continue improving branch.
I'll post cluster-revised PR to finally merge to 3.0.0.

[marcosnils]

@HeartSaVioR perfect.

[HeartSaVioR]

@marcosnils @phufool It's ready to serve at #671.
I also updated cluster-revised branch to follow up current master.

[phufool]

Awesome! Looking forward to it!