[ZEPPELIN-1233] Support Redis interpreter #1497

darionyaphet · 2016-10-08T14:20:10Z

What is this PR for?

Support Redis interpreter

What type of PR is it?

Feature

Todos

- Support Redis Cluster

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1233

How should this be tested?

Outline the steps to test the PR here.

Screenshots (if appropriate)

Questions:

Does the licenses files need update?
Is there breaking changes for older versions?
Does this needs documentation?

anthonycorbacho

Thank you for bringing Redis Interpreter! this is awesome!

I did a quick review on your code please check it and let me know what you think.

anthonycorbacho · 2016-10-08T14:44:26Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+
+    private Jedis client;
+
+    private enum RedisCommand {


As a redis user, i am expecting to use other redis command like hkeys, range and stuff.
Maybe its a good idea to add more here or come up with a better approach ?

Actually I'm not sure is there any good way to running redis command so I just simply create GET and SET :)

maybe I can use redis.clients.jedis.Protocol.Command to replacement RedisCommand

yes, i think its better to use this enum
i was thinking about doing something like:

private final HashSet<String> redisCommands; public static HashSet<String> getRedisCommands() { HashSet<String> hashSetOfRedisCommands = new HashSet<String>(); for (Command c : Command.values()) { hashSetOfRedisCommands.add(c.name()); } return hashSetOfRedisCommands; } public RedisInterpreter(Properties property) { super(property); redisCommands = getRedisCommands(); } // then use this like redisCommands.contains(tokens[1]);

what do you think?

Maybe we don't need a redis commands set to check whether the input command is illegal because there is a default case in switch . Is it a better one?

String[] tokens = splitTokens(input); String keyword = tokens[0]; switch (Protocol.Command.valueOf(keyword)) { case GET: result = getProcess(); break; case SET: result = setProcess(); break; default: result = new InterpreterResult(ERROR, "key word not found"); break; }

Yes it look better :)

anthonycorbacho · 2016-10-08T14:48:40Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+
+    @Override
+    public void open() {
+        client = new Jedis();


I think here we should provide a better Redis client creation, as far as i know, this will assume that you are running redis locally, i think we can exploit more the interpreter setting creation, here an example with JDBC.
So user will be able to set host, port and credential.

Also how about using JedisPool instead of Jedis?

It will look like something like this

private static final int MAX_CONNECTION = 142; private static final int MAX_IDLE = 10; private static final int MIN_IDLE = 142; private static final long MAX_WAIT = TimeUnit.SECONDS.toMillis(30); private JedisPool jedisPool; public void open() { jedisPool = new JedisPool(getPoolConfiguration(), property.getCacheHost(), property.getCachePort()); } private JedisPoolConfig getPoolConfiguration() { JedisPoolConfig config = new JedisPoolConfig(); config.setMaxTotal(MAX_CONNECTION); config.setMaxIdle(MAX_IDLE); config.setMinIdle(MIN_IDLE); config.setMaxWaitMillis(MAX_WAIT); return config; } // Utils method :) private Jedis borrow() { return jedisPool.getResource(); } // Then you can use it like private InterpreterResult getProcess(String key) { Jedis jedis = borrow(); Pipeline pipe = jedis.pipelined(); Response<String> response = pipe.get(key); pipe.sync(); jedis.close(); String result = response.get(); return new InterpreterResult(SUCCESS, result); }

I read Jedis source code and find a strange case . The returnResource will be deprecated after 3.0 and the resource shoule be close after usage (don't return into source pool ) . Now I'm using 2.9 . I'm not sure it's a good idea .

Maybe using zeppelin to access Redis will not very frequently so create jedis client after it close is OK .

I think its better to use a Pool for this job, you only need jedis instance when you actually run the jobs, that is whay i am asking for changing from Redis to JedisPool, and in the doc i didnt find the notice about removing returnResource.

anthonycorbacho · 2016-10-08T14:51:21Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

@@ -0,0 +1,98 @@
+package org.apache.zeppelin.redis;


ASF Header is missing here.

anthonycorbacho · 2016-10-08T14:52:11Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+import static org.apache.zeppelin.interpreter.InterpreterResult.Code.ERROR;
+import static org.apache.zeppelin.interpreter.InterpreterResult.Code.SUCCESS;
+
+public class RedisInterpreter extends Interpreter {


Could you also add the class header with a description of this interpreter?
This will help futur contributors.

anthonycorbacho · 2016-10-08T14:53:04Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+
+public class RedisInterpreter extends Interpreter {
+
+    private Logger LOG = LoggerFactory.getLogger(RedisInterpreter.class);


Can we change this field for private static final Logger LOG = LoggerFactory.getLogger(RedisInterpreter.class);?

this will help to have one logger instance :)

felixcheung · 2016-10-09T18:09:09Z

Hi - thank you for your contribution. Could you see the list of steps here:
https://zeppelin.apache.org/docs/0.6.1/development/writingzeppelininterpreter.html#contributing-a-new-interpreter-to-zeppelin-releases

Notably, could you add:

tests
documentation
list licenses for dependencies

anthonycorbacho · 2016-10-12T00:34:42Z

@darionyaphet do you need some help for the change?

darionyaphet · 2016-10-12T07:18:23Z

Hi @anthonycorbacho thank you for you great help and I'm still working on it .

I have implement a lot of command include GEO ,HASH, HYPERLOGLOG, LIST, KEYS, Sets ,SORTED SET and Strings and have updated .

anthonycorbacho · 2016-10-14T07:45:54Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+    String keyword = tokens[0];
+
+    InterpreterResult result;
+    switch (Protocol.Command.valueOf(keyword)) {


Humm, that is a very massive switch case, i think we can come up with a more elegant way to to this kind of things,

since it mainly take the same argument String[] token, i guess we can go with the design pattern Strategy Pattern.

This idea here is to extract the logic to smaller classes and register then to a map.

eg:

// In the constructor Map<Protocol.Command, Command> redisCommand = Maps.newHashMap<>(); // register command and class redisCommand.put(GEOADD, GeoAdd.createInstance()); ... // add other command // create a java package org.apache.zeppelin.redis.cmd and add every implementation of redis cmd. // but first you need to create a interface // eg: public interface Cmd { public boolean execute(String[] tokens); } // use this interface to implement the actual cmd public class MyRedisCmd implements Cmd { public static MyRedisCmd createInstance() { return new MyredisCmd(); } @Override public boolean execute(String[] tokens) { // do the work here } } // Once you have implemented the redis cmd, you can use this like this: redisCommand.get(Protocol.Command.valueOf(keyword)).execute(tokens);

let me know what you think

@anthonycorbacho @kavinkumarks thanks for you reply with great suggest 👍 . Using Strategy Pattern to execute Redis Command will take me make a lot of classes , each Redis Command should have a class to express it . I'm not sure it's a good idea maybe the long switch list is more clearly to execute the simple redis commands in a single Java class .

what do you think :)

PS : I will append some test case about Redis Command later .

AFAIK, what @anthonycorbacho suggested seems more like a command pattern. (since it is about the action - what to do, not the algorithm - how to do)
Anyway +1 to apply what @anthonycorbacho suggested. Having more classes is not worse than having too long single class, (and also too long switch statement) especially they can be organized by package. We can even use static inner class to reduce the file count if we really want.

Btw, to be clear, I was the other one who want to make the Redis interpreter but at the design phase I was stuck on converting Redis command statement to Jedis method call.
(I was struggling to find a way to not having static mapping like this, but that was not easy as I thought.)
I will come up with clearer way sooner or later if possible.

Personally, I would prefer to have classes (will make test more easy) than a massive switch case. the main concern i have about big switch case tis that it is really hard to maintain and make the parent class huge. by de coupling the logic into smaller classes will improve the maintainability and make testability more easy.

@anthonycorbacho @HeartSaVioR It's seems a good idea to split execute Redis Command into classes and this command classes will reused in Redis Cluster Interpreter. I have create a abstract class RedisCommand as a basic class :

public abstract class RedisCommand { private JedisPool clientPool; public RedisCommand(JedisPool clientPool) { this.clientPool = clientPool; } private Jedis borrowClient() { return clientPool.getResource(); } private void releaseClient(Jedis client) { client.close(); } public InterpreterResult execute(String[] tokens) { Jedis client = borrowClient(); try { return execute(client, tokens); } catch (JedisException exception) { return new InterpreterResult(ERROR, exception.getMessage()); } finally { releaseClient(client); } } /** * @param tokens * @return * @throws Exception */ protected abstract InterpreterResult execute(Jedis client, String[] tokens); }

I think it's OK and each redis command could extend this one .

kavinkumarks · 2016-10-14T13:13:20Z

I too checked the changes and they look good.The comment from @anthonycorbacho about usage of "Strategy pattern" for selecting the relevant redis command instance and executing them is the better way to implement it.

Also, could we add some test cases for this?

Thanks,
Kavin
MailTo: [email protected]

HeartSaVioR · 2016-10-14T23:03:25Z

Every command methods have a chance to leak Jedis instance when command is failing and throwing a kind of JedisException.
You need to replace your borrow-call-return pattern with try-with-resource thanks to Zeppelin requires Java 1.7.

anthonycorbacho · 2016-10-16T04:51:34Z

@HeartSaVioR thank you for pointing that out, you are right we better handle the potential JedisException exception to avoid leak.

darionyaphet · 2016-10-18T09:48:25Z

Hi @anthonycorbacho @HeartSaVioR I have update RedisInterpreter with some redis commands (include APPEND ,BITCOUNT ,BITFIELD ,BITOP, BITPOS, DECR) .

Please help review this one . If anything is OK , I will clean the commented code and completion other redis commands . Thanks ~~ :)

anthonycorbacho

Great to see this interpreter improving day by day!!
@HeartSaVioR what do you think?

anthonycorbacho · 2016-10-21T08:31:15Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+        return (String[]) list.toArray();
+    }
+
+//    ////////////////////////// KEYS //////////////////////////


Better to remove comment here if there are not intentions of using the code below.

Hi I still working on Key commands . I will clean the unnecessary code after finished this PR :)

anthonycorbacho · 2016-10-21T08:33:22Z

redis/src/main/java/org/apache/zeppelin/redis/command/RedisCommand.java

+        try {
+            return new InterpreterResult(SUCCESS, execute(client, tokens));
+        } catch (JedisException exception) {
+            return new InterpreterResult(ERROR, exception.getMessage());


What do you think about adding a LOG.error here? something like
LOG.error("Couldn't execute redis command {}", token[0], e);

anthonycorbacho · 2016-10-21T08:34:35Z

redis/src/main/resources/interpreter-setting.json

+      }
+    }
+  }
+]


Need a new line here :)

anthonycorbacho · 2016-10-21T08:34:39Z

redis/src/test/java/org/apache/zeppelin/redis/RedisInterpreterTest.java

+      client.close();
+    }
+  }
+}


Need a new line here :)

anthonycorbacho · 2016-10-21T08:36:39Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+        if (COMMANDS.containsKey(command)) {
+            result = COMMANDS.get(command).execute(tokens);
+        } else {
+            result = new InterpreterResult(ERROR, "keyword not found");


Maybe we can provide a better error to the user like
Keyword " + tokens + " is not supportted?

What do you think?

Yep It's a good idea 👍

HeartSaVioR

I left review comments but overall direction looks good unless we find brilliant idea to reduce verbose.

Please note that I couldn't check actual call of Jedis method since they're too many. I would like to see all tests instead, but tests don't necessary need to have files for each command.

Some of Jedis methods can return null (for example, if there's no matching result) which should be handled properly. Some of implementations of RedisCommands might throw NPE in some cases since you manipulate the result without null check. I'm not sure interpreter result type NULL is for these case.
@anthonycorbacho Could you clarify above this?
May be better to have generic return type for RedisCommand, and let RedisInterpreter handles null and also type-aware result.

HeartSaVioR · 2016-10-22T03:39:11Z

conf/interpreter-list

 md              org.apache.zeppelin:zeppelin-markdown:0.6.1             Markdown support
 postgresql      org.apache.zeppelin:zeppelin-postgresql:0.6.1           Postgresql interpreter
 python          org.apache.zeppelin:zeppelin-python:0.6.1               Python interpreter
+redis           org.apache.zeppelin:zeppelin-redis:0.6.0                Redis interpreter


version mismatch

HeartSaVioR · 2016-10-22T03:40:23Z

redis/pom.xml

+  <version>0.7.0-SNAPSHOT</version>
+
+  <properties>
+    <redis.version>2.9.0</redis.version>


It can make unneeded confusing. jedis.version would be better.

HeartSaVioR · 2016-10-22T03:42:58Z

redis/src/main/java/org/apache/zeppelin/redis/RedisInterpreter.java

+        LOG.info("Connection to %s:%d", host, port);
+
+        // TODO(darion) : Need more config ?
+        config = new JedisPoolConfig();


Since Redis interpreter doesn't need to be super fast at all, testOnBorrow() can relieve the connection issue with ignorable overhead.

HeartSaVioR · 2016-10-22T03:50:15Z

redis/src/main/java/org/apache/zeppelin/redis/command/RedisCommand.java

+import static org.apache.zeppelin.interpreter.InterpreterResult.Code.SUCCESS;
+
+/**
+ * Redis BLPOPCommand


Wrong explanation here.

HeartSaVioR · 2016-10-22T03:52:08Z

redis/src/main/java/org/apache/zeppelin/redis/command/RedisCommand.java

+    }
+
+    public InterpreterResult execute(String[] tokens) {
+        Jedis client = borrowClient();


You can use try-with-resource:

try (Jedis client = clientPool.getResource()) { ... }

No need to have borrowClient() and releaseClient(), and also no need to explicitly handling closing resource.

HeartSaVioR · 2016-10-22T03:52:26Z

redis/src/main/java/org/apache/zeppelin/redis/command/RedisCommand.java

+        try {
+            return new InterpreterResult(SUCCESS, execute(client, tokens));
+        } catch (JedisException exception) {
+            return new InterpreterResult(ERROR, exception.getMessage());


HeartSaVioR · 2016-10-22T03:56:18Z

redis/src/main/java/org/apache/zeppelin/redis/command/geo/GEOADDCommand.java

+
+    @Override
+    protected String execute(Jedis client, String[] tokens) {
+        String result = client.geoadd(tokens[1], Double.valueOf(tokens[2]),


I'd prefer wrapping it with String.valueOf() or call toString() if it's same, but this is simpler so OK to me.

And I also prefer returning it without having local variable (this style occurs all of RedisCommand implementations), but this way makes it easier to set a breakpoint. I'd like to see other's opinions here.

toString maybe better and reasonable . @HeartSaVioR

HeartSaVioR · 2016-10-22T04:14:22Z

redis/src/main/java/org/apache/zeppelin/redis/command/list/BLPOPCommand.java

+
+    @Override
+    protected String execute(Jedis client, String[] tokens) {
+        String[] keys = new String[tokens.length - 2];


Please extract this logic to method of utility class or method of RedisCommand. This is duplicated from many places. The logics which manipulate keys/parameters are all candidates to extract.

anthonycorbacho · 2016-10-22T12:06:45Z

@HeartSaVioR yeah i would love to have a class that manipulate redis cmd and handle null, ussually i am not a huge fan of returning null, i would prefer to return an EMPTY type instead.

darionyaphet · 2016-10-27T09:02:32Z

Aha. A lot of files conflict and I can't resolve that . Maybe I will close this one and reopen another .

HeartSaVioR · 2016-10-27T09:09:54Z

Follow this if rebase with master is hard due to lots of conflicts:

create a new branch which is based on current master
cherry-pick your commits (write down your last commit id)
change branch to PR's branch
git reset --hard last commit id
git push --force

last commit id might be replaced with branch name you created, but I didn't try it.

darionyaphet · 2016-11-04T13:58:57Z

Add a Screenshot

Support Redis interpreter Feature Support Redis Cluster - https://issues.apache.org/jira/browse/ZEPPELIN-1233 Outline the steps to test the PR here. - Does the licenses files need update? - Is there breaking changes for older versions? - Does this needs documentation?

close #83 close #86 close #125 close #133 close #139 close #146 close #193 close #203 close #246 close #262 close #264 close #273 close #291 close #299 close #320 close #347 close #389 close #413 close #423 close #543 close #560 close #658 close #670 close #728 close #765 close #777 close #782 close #783 close #812 close #822 close #841 close #843 close #878 close #884 close #918 close #989 close #1076 close #1135 close #1187 close #1231 close #1304 close #1316 close #1361 close #1385 close #1390 close #1414 close #1422 close #1425 close #1447 close #1458 close #1466 close #1485 close #1492 close #1495 close #1497 close #1536 close #1545 close #1561 close #1577 close #1600 close #1603 close #1678 close #1695 close #1739 close #1748 close #1765 close #1767 close #1776 close #1783 close #1799

darionyaphet force-pushed the ZEPPELIN-1233 branch from cca1102 to adb7818 Compare October 8, 2016 14:22

anthonycorbacho suggested changes Oct 8, 2016

View reviewed changes

darionyaphet force-pushed the ZEPPELIN-1233 branch 2 times, most recently from f40e716 to ecfa1b2 Compare October 9, 2016 09:45

darionyaphet force-pushed the ZEPPELIN-1233 branch 2 times, most recently from 39e42ef to 244fe85 Compare October 12, 2016 07:16

darionyaphet force-pushed the ZEPPELIN-1233 branch from 244fe85 to 5b60b8a Compare October 12, 2016 07:21

anthonycorbacho reviewed Oct 14, 2016

View reviewed changes

darionyaphet force-pushed the ZEPPELIN-1233 branch 2 times, most recently from 5ad9542 to 87a22cb Compare October 18, 2016 09:44

darionyaphet force-pushed the ZEPPELIN-1233 branch 2 times, most recently from e1c2f6c to 3b93b3e Compare October 20, 2016 14:05

anthonycorbacho reviewed Oct 21, 2016

View reviewed changes

HeartSaVioR reviewed Oct 22, 2016

View reviewed changes

darionyaphet force-pushed the ZEPPELIN-1233 branch 2 times, most recently from 4ecd614 to 2937c92 Compare October 27, 2016 08:53

darionyaphet force-pushed the ZEPPELIN-1233 branch 4 times, most recently from 472c22d to d319009 Compare October 27, 2016 10:07

darionyaphet force-pushed the ZEPPELIN-1233 branch 5 times, most recently from 3e8a5c2 to 03cd13a Compare October 27, 2016 10:49

darionyaphet force-pushed the ZEPPELIN-1233 branch from ee3572c to 5204881 Compare November 4, 2016 14:16

darionyaphet force-pushed the ZEPPELIN-1233 branch from 477be1a to 0f3cba5 Compare November 4, 2016 14:24

asfgit closed this in c38a0a0 May 9, 2018


		public class RedisInterpreter extends Interpreter {

		private Logger LOG = LoggerFactory.getLogger(RedisInterpreter.class);

+                    }
+                  }
+                }
+              ]

                
                    No newline at end of file

[ZEPPELIN-1233] Support Redis interpreter #1497

[ZEPPELIN-1233] Support Redis interpreter #1497

Uh oh!

Conversation

darionyaphet commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

anthonycorbacho left a comment

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darionyaphet Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Oct 9, 2016

Uh oh!

anthonycorbacho commented Oct 12, 2016

Uh oh!

darionyaphet commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darionyaphet Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kavinkumarks commented Oct 14, 2016

Uh oh!

HeartSaVioR commented Oct 14, 2016

Uh oh!

anthonycorbacho commented Oct 16, 2016

Uh oh!

darionyaphet commented Oct 18, 2016

darionyaphet commented Oct 8, 2016 •

edited

Loading

anthonycorbacho Oct 8, 2016 •

edited

Loading

anthonycorbacho Oct 9, 2016 •

edited

Loading

darionyaphet Oct 9, 2016 •

edited

Loading

anthonycorbacho Oct 8, 2016 •

edited

Loading

anthonycorbacho Oct 8, 2016 •

edited

Loading

anthonycorbacho Oct 8, 2016 •

edited

Loading

darionyaphet Oct 16, 2016 •

edited

Loading

HeartSaVioR left a comment •

edited

Loading