fix #106 Gearpump Redis Integration #11

darionyaphet · 2016-05-07T12:18:17Z

Redis is a hight performance in memory storage , and have widely used in a lot of project .

It's should support redis as DataSource and DataSink .

kkasravi · 2016-05-07T17:32:59Z

@darionyaphet we probably need some unit tests. Good job on splitting example and actual definitions into example and external. It doesn't look like the build failures are related

[info] StreamAppSpec:
[info] - should be able to generate multiple new streams
[info] - should plan the dsl to Processsor(TaskDescription) DAG *** FAILED ***
[info]   java.io.NotSerializableException: scala.collection.LinearSeqLike$$anon$1
[info]   at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)
[info]   at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
[info]   at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
[info]   at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
[info]   at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
[info]   at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply$mcV$sp(Serializer.scala:235)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply(Serializer.scala:235)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply(Serializer.scala:235)
[info]   at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
[info]   ...
[info] - should produce 3 messages *** FAILED ***
[info]   java.io.NotSerializableException: scala.collection.LinearSeqLike$$anon$1
[info]   at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)
[info]   at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
[info]   at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
[info]   at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
[info]   at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
[info]   at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply$mcV$sp(Serializer.scala:235)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply(Serializer.scala:235)
[info]   at akka.serialization.JavaSerializer$$anonfun$toBinary$1.apply(Serializer.scala:235)
[info]   at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
[info]   ...

manuzhang · 2016-05-08T04:44:31Z

@darionyaphet thanks for your contributions. please add license headers

darionyaphet · 2016-05-08T15:20:01Z

@manuzhang I have add license headers .

@kkasravi I will add some unit tests .

thank you :)

huafengw · 2016-05-11T01:41:01Z

@darionyaphet we have changed Gearpump's package name, please rebase our latest code. Sorry for the inconvenient.

manuzhang · 2016-05-11T01:46:28Z

@darionyaphet also please modify your commit comment to fix GEARPUMP-106, Redis integration which would make our pr merge script happier.

darionyaphet · 2016-05-11T03:48:36Z

Hi @manuzhang what is the commit format ?

I found some rules on Contribution Guideline

For all commit log messages, they must contain issue id. Like this: "fix #issueId, comments".

darionyaphet · 2016-05-11T03:54:24Z

Hi @huafengw sure I will rename packages from io.gearpump to org.apache.gearpump :D

manuzhang · 2016-05-11T03:59:12Z

@darionyaphet yes, as we use jira now issue id should be GEARPUMP-XXX. You may our recent commits for examples

manuzhang · 2016-05-11T12:07:29Z

...aming/redis/src/main/scala/org/apache/gearpump/streaming/example/redis/RedisSourceSink.scala

+  }
+}
+
+object RedisSourceSink extends AkkaApp with ArgumentsParser {


RedisSourceSink doesn't sound like an example and I can't get what the example does from the name.

RedisSourceSink which is mean reading from RedisSource and write messages to RedisSink :)

So maybe name RedisSourceSinkExample

manuzhang · 2016-05-11T12:24:09Z

@darionyaphet Could you add this module to the experiments ? That will be low risk before the module is mature. Plus, the experiments doesn't require UT coverage. Also, please add a README for how to use this module.

kkasravi · 2016-05-11T15:02:56Z

...aming/redis/src/main/scala/org/apache/gearpump/streaming/example/redis/RedisSourceSink.scala

+  import taskContext.output
+
+  override def onNext(message: Message): Unit = {
+    val msg = message.msg.asInstanceOf[Option[String]]


this would be better as

Option(message.msg) match { case Some(msg) => val upper = msg.asInstanceOf[String].toUpperCase LOG.info(s"to Upper $upper") outer(new Message(upper, message.timestamp)) case None => }

Good ! I will update this :)

huafengw · 2016-05-13T03:16:15Z

Though the new package name is right, you still need to rebase latest master branch to correct the import.
Also, please add some comment to explain the interfaces and classes.

kkasravi · 2016-05-13T14:20:02Z

@darionyaphet if you are time constrained please let us know and we can make the changes noted above. We would like to get this into the next release if possible (we're targeting in about 2 weeks).
thanks
Kam

darionyaphet · 2016-05-14T15:06:52Z

Sorry to reply it with so long .

@manuzhang @huafengw redis examples class name have rename to RedisSourceSinkExample and RedisSourceStorageExample , also add into BuildExample.scala . It seems better ?

external-redis have move into experiments I will add unit test later :)

@kkasravi I will fix and add some comment at weekend :)

huafengw · 2016-05-19T02:39:28Z

project/BuildExample.scala

+          CrossVersion.binaryScalaVersion(scalaVersion.value)
+      ) ++
+      Seq(
+        mainClass in (Compile, packageBin) := Some("org.apache.gearpump.streaming.example.redis.RedisSourceStorageExample"),


That's not correct, one manifest can only have one main class.

Seq( mainClass in (Compile, packageBin) := Some("org.apache.gearpump.streaming.example.redis.RedisSourceSinkExample"), target in assembly := baseDirectory.value.getParentFile.getParentFile / "target" / CrossVersion.binaryScalaVersion(scalaVersion.value) )

is enough.

HI @huafengw when I package using SBT , the main class will specifies into META-INF ?

Yeah, it's what sbt-pack did.

manuzhang · 2016-06-01T08:19:17Z

experiments/redis/src/main/scala/org/apache/gearpump/streaming/redis/RedisMessage.scala

+    def this(key: String, value: String) = this(toBytes(key), toBytes(value))
+  }
+
+  case class LPushMessage(key: Array[Byte], value: Array[Byte]) {


It's unclear how this message type can be used. LPushMessage looks the same as RPushMessage

LPushMessage and RPushMessage are difference message to control redis client add element at list's head or tail

manuzhang · 2016-06-06T05:24:06Z

@darionyaphet What's the difference between RedisStorage and RedisSink here since they both extend DataSink ?

karol-brejna-i · 2016-06-16T09:34:14Z

I think I see the idea behind RedisSink and RedisStorage.

Redis is advertised as "data structure store". It supports structures (like strings, lists, sets, hashes, etc.) and operation on them (add element to a set, remove element; add element at the beginning of a list, at the end; remove element from head/tail of the list, etc.).
It also pub/sub server (http://redis.io/topics/pubsub)

RedisSource and RedisSink work on "classic" message stream utilizing Redis' pub/sub mechanizm. The source subscribes to a channel. The sink publishes to a channel.

RedisStorage reads a message and let you make a Redis command (add to list, set, etc.). The subset of commands is defined in RedisMessage. (I would think about changing the name, though. For example RedisCommandSink or similar).

@manuzhang what do you think?

codecov-io · 2016-06-20T14:53:59Z

Current coverage is 64.10%

No coverage report found for master at 25d1fce.

Powered by Codecov. Last updated by 25d1fce...c3686fd

darionyaphet · 2016-06-20T15:54:48Z

@manuzhang actually storm-redis act as a ORM framework and put the message into a redis instance . So it don't have a full command support . I think we can support more command such as delete , ttl and exists .

karol-brejna-i · 2016-06-20T18:43:01Z

We are developing under 'experiments' here. I'd be happy to have something decent here. Maybe not full-blown, but functional.
Can we sum up what we still need to do with this PR?
Putting the examples in experiments folder?
Tests?
Updating names?

My point is: Let us release first version of the redis connector and improve it in next releases.

manuzhang · 2016-06-21T01:16:45Z

@karol-brejna-i @darionyaphet I understand the difference between RedisSink and RedisStorage now but I don't think RedisStorage should extend the DataSink interface which is intended for write. Probably we need a new interface for such usage as RedisStorage. For now, we can add RedisStorage without extending any interface. I totally agree we have a functional version first and improve later as long as all are put in the experiments folder with reasonable naming. Tests are not hard requirement for a experiments module.

darionyaphet · 2016-06-22T01:06:32Z

@karol-brejna-i It's seems have a lot things to do .

Support more command such as : time to live , exist and delete .
Support Redis Cluster and Redis Sharding .
Add some Test Case

darionyaphet · 2016-06-22T01:09:53Z

@manuzhang

HBaseSink seems have the same question .

Sink and Source are support to the data streaming , Storage is meaning to put data into a place .

karol-brejna-i · 2016-06-22T18:09:48Z

@darionyaphet

Support more command such as : time to live , exist and delete .
This is the reason I tend to agree @manuzhang. Let's have the "basic" source/sink. Let's make RedisStorage a helper class (not a DataSink). This will let us release a functional version of the connector and improve later.

Support Redis Cluster and Redis Sharding .
Looks like Jedis has cluster support already (https://github.com/xetorthio/jedis#jedis-cluster). Sharding, too. So it would be a matter of using the features. Again, I would try to incorporate the features in next releases.

Add some Test Case
As Manu wrote, test are not the blocker for an experiment module.

My message is: let's release first version of Redis Connector soon and give ourselves time to improve ;-)

darionyaphet · 2016-07-01T13:55:10Z

@kkasravi @manuzhang RedisStorage maybe need more discuss so I have remove it .

After we have a discuss , I will repush it :)

manuzhang · 2016-12-09T03:10:52Z

@darionyaphet this is partially done in #93. Please rebase or, close and fire a new PR for follow-ups.

update the rest api

Author: Kam Kasravi <[email protected]> Closes apache#11 from kkasravi/asf-site.

manuzhang reviewed May 11, 2016
View reviewed changes

kkasravi reviewed May 11, 2016
View reviewed changes

darionyaphet added 3 commits May 13, 2016 12:47

fix apache#106 Gearpump Redis Integration

bfa4702

add license header

e904d65

fix GEARPUMP-106, rename package

376bd85

fix GEARPUMP-106 rename package name and move to experiments

64811ca

darionyaphet force-pushed the GEARPUMP-106 branch from d9304ef to 64811ca Compare May 14, 2016 15:08

darionyaphet added 3 commits May 15, 2016 11:51

fix GEARPUMP-106, remove duplicate examples

f6d7be3

fix GEARPUMP-106, remove redis in external module

123439b

fix GEARPUMP-106, update message check

981178e

huafengw reviewed May 19, 2016
View reviewed changes

manuzhang reviewed Jun 1, 2016
View reviewed changes

GEARPUMP-106 : Add some comment and README

c3686fd

remove redis storage

19febe0

manuzhang pushed a commit to manuzhang/incubator-gearpump that referenced this pull request Jan 24, 2017

Merge pull request apache#11 from skw1992/update_rest_api

e861264

update the rest api

manuzhang pushed a commit to manuzhang/incubator-gearpump that referenced this pull request Jan 24, 2017

[GEARPUMP-242] Update website for 0.8.2-incubating release

5fca623

Author: Kam Kasravi <[email protected]> Closes apache#11 from kkasravi/asf-site.

manuzhang pushed a commit to manuzhang/incubator-gearpump that referenced this pull request Jan 24, 2017

[GEARPUMP-242] Update website for 0.8.2-incubating release

0d2d025

Author: Kam Kasravi <[email protected]> Closes apache#11 from kkasravi/asf-site.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #106 Gearpump Redis Integration #11

fix #106 Gearpump Redis Integration #11

darionyaphet commented May 7, 2016

kkasravi commented May 7, 2016

manuzhang commented May 8, 2016

darionyaphet commented May 8, 2016

huafengw commented May 11, 2016

manuzhang commented May 11, 2016

darionyaphet commented May 11, 2016

darionyaphet commented May 11, 2016

manuzhang commented May 11, 2016

manuzhang May 11, 2016

darionyaphet May 11, 2016

huafengw May 13, 2016

manuzhang commented May 11, 2016

kkasravi May 11, 2016

darionyaphet May 14, 2016

huafengw commented May 13, 2016

kkasravi commented May 13, 2016

darionyaphet commented May 14, 2016

huafengw May 19, 2016 •

edited

Loading

darionyaphet May 19, 2016

huafengw May 19, 2016

manuzhang Jun 1, 2016

darionyaphet Jun 2, 2016

manuzhang commented Jun 6, 2016

karol-brejna-i commented Jun 16, 2016 •

edited

Loading

codecov-io commented Jun 20, 2016

darionyaphet commented Jun 20, 2016

karol-brejna-i commented Jun 20, 2016

manuzhang commented Jun 21, 2016

darionyaphet commented Jun 22, 2016

darionyaphet commented Jun 22, 2016

karol-brejna-i commented Jun 22, 2016

darionyaphet commented Jul 1, 2016

manuzhang commented Dec 9, 2016

fix #106 Gearpump Redis Integration #11

Are you sure you want to change the base?

fix #106 Gearpump Redis Integration #11

Conversation

darionyaphet commented May 7, 2016

kkasravi commented May 7, 2016

manuzhang commented May 8, 2016

darionyaphet commented May 8, 2016

huafengw commented May 11, 2016

manuzhang commented May 11, 2016

darionyaphet commented May 11, 2016

darionyaphet commented May 11, 2016

manuzhang commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuzhang commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huafengw commented May 13, 2016

kkasravi commented May 13, 2016

darionyaphet commented May 14, 2016

Sorry to reply it with so long .

huafengw May 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuzhang commented Jun 6, 2016

karol-brejna-i commented Jun 16, 2016 • edited Loading

codecov-io commented Jun 20, 2016

Current coverage is 64.10%

darionyaphet commented Jun 20, 2016

karol-brejna-i commented Jun 20, 2016

manuzhang commented Jun 21, 2016

darionyaphet commented Jun 22, 2016

darionyaphet commented Jun 22, 2016

karol-brejna-i commented Jun 22, 2016

darionyaphet commented Jul 1, 2016

manuzhang commented Dec 9, 2016

huafengw May 19, 2016 •

edited

Loading

karol-brejna-i commented Jun 16, 2016 •

edited

Loading