add redis channel #176

JunLuo · 2017-09-16T13:39:43Z

In some cases, we need events can not be lost. But we don't want to install zk & kafka.
To make event deliver only once and to deploy many flume agents in different nodes but share only one channel, we developed redis channel.
We already use the redis channel in out production environment, and we want to feed back to flume.

add redis channel

bessbd · 2017-09-16T15:34:11Z

Hi @JunLuo ,

Thanks for the pull request!

This looks like a very useful change to me. However, Travis CI has reported a compilation error:

[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[3,22] error: package junit.framework does not exist
[ERROR] /home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[4,22] error: package junit.framework does not exist
[ERROR] /home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[5,22] error: package junit.framework does not exist
[ERROR] /home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[11,12] error: cannot find symbol
[ERROR] class TestCase
/home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[26,18] error: cannot find symbol
[ERROR] class AppTest
/home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[28,19] error: cannot find symbol
[ERROR] class AppTest
/home/travis/build/apache/flume/flume-ng-channels/flume-redis-channel/src/test/java/org/flume/redis/channel/AppTest.java:[36,8] error: cannot find symbol
[INFO] 7 errors

Can you please fix it, so that we can proceed with the reviews?

Thank you,

Donat

delete unused testapp

add license comments

fix codestyle check errors

init redis channel version

fix a stupid error

JunLuo · 2017-09-16T23:46:04Z

Hi @bessbd ,
I have fixed the errors about Junit and code style :)

fix a bug

bessbd · 2017-09-25T21:35:24Z

Hi @JunLuo ,

Thank you for the changes!

On a first pass, I've noticed that a new user guide entry is missing.
For this change to get merged, some documentation would be necessary.
For an example, I'd recommend Kafka Channel.
Do you think you can make the necessary additions?

Thank you,

Donat

add redis channel documentation

JunLuo · 2017-09-26T10:43:53Z

Hi @bessbd ,
I've add some documentation about redis channel :)

szaboferee

Thank you for the new channel, it is going to be very useful!
I added some questions and suggestions bellow.

It would be great if you could write unit and/or integration test.
https://github.com/kstyrc/embedded-redis
Could be a good tool to create the integration tests.

szaboferee · 2017-09-26T14:17:50Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+ */
+package org.apache.flume.channels.redis;
+
+import com.google.common.base.Optional;


Is it possible to use java 8 optional instead?

szaboferee · 2017-09-26T14:23:28Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+    @Override
+    protected void doPut(Event event) throws InterruptedException {
+      type = TransactionType.PUT;
+      if (!serializedEvents.isPresent()) {


Why not just initialize with the empty list?

or we could use the Optional.ifPresent(Consumer<? super T> consumer) at the end, where the get() is called

szaboferee · 2017-09-26T14:33:07Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+      }
+
+      try {
+        if (!tempOutStream.isPresent()) {


the optional wrapper seems unnecessary because there is a single initialization and 3 get in the same method.
Initialization could be moved to declaration

szaboferee · 2017-09-26T14:36:21Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+        if (!tempOutStream.isPresent()) {
+          tempOutStream = Optional.of(new ByteArrayOutputStream());
+        }
+        if (!writer.isPresent()) {


the optional might be an overhead here too.

szaboferee · 2017-09-26T14:50:23Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+      } catch (Exception ex) {
+        LOGGER.warn("Error while get multi message", ex);
+      }
+      if (!events.isPresent()) {


This variable has only this initialization and a few get in this class. I guess the Optional here is not needed.

szaboferee · 2017-09-26T16:04:58Z

flume-ng-doc/sphinx/FlumeUserGuide.rst

+.. code-block:: properties
+
+    a1.channels.channel1.type = org.flume.channels.redis.RedisChannel
+    a1.channels.channel1.server = redis-server


It might be better to have an example host (localhost) here or an IP address

szaboferee · 2017-09-26T16:06:46Z

flume-ng-doc/sphinx/FlumeUserGuide.rst

+
+================================================  ================================  ========================================================
+Property Name                                     Default                           Description
+================================================  ================================  ========================================================


The cluster scenario is not documented

szaboferee · 2017-09-26T16:08:09Z

...flume-redis-channel/src/main/java/org/apache/flume/channels/redis/tools/RedisController.java

+  private int maxWaitMillis;
+
+  private JedisPoolConfig createJedisConfig() {
+    redisConnTimeout = 60000;


It would be great if these settings could be set from the configuration

szaboferee · 2017-09-26T16:09:24Z

...flume-redis-channel/src/main/java/org/apache/flume/channels/redis/tools/RedisController.java

+    return jedisConfig;
+  }
+
+  public static RedisController getRedisController(String host, int port,


What happens if we would like to have two channel with different redis servers?

szaboferee · 2017-09-26T16:10:28Z

...flume-redis-channel/src/main/java/org/apache/flume/channels/redis/tools/RedisController.java

+    maxIdle = 300;
+    minIdle = 10;
+    maxWaitMillis = 60000;
+    JedisPoolConfig jedisConfig = new JedisPoolConfig();


I see the cluster related config names but not the actual implementation that uses them.

remove useness optional wrapper

`add sentinel and cluster type `move redis config to channel config `add unit test

fix code style

fix code style error

add cluster and redis config documention

JunLuo · 2017-09-29T14:03:42Z

Hi @szaboferee ,
I've done these changes:

Add cluster and sentinel mode
Remove useless optional
Add the documention about cluster and sentinel
Move redis config to flume configuration
Fix the error of singleton
Add unit test

JunLuo · 2017-10-28T02:44:47Z

Hi @szaboferee ，
I've fix all problems you mentions in commits, What should I do next?

bessbd · 2017-10-29T12:27:28Z

flume-ng-doc/sphinx/FlumeUserGuide.rst

+Redis Channel
+~~~~~~~~~~~~~
+
+The events are stored in a redis server (must be installed separately). Redis provides high concurrency. If you have many agents but want to share one channel which can provide high concurrency, Redis Channel is a good choice. Redis also can provide high availability and replication, you can choose the best way to adapt your application.


Should be "Redis" instead of "redis".

Also, the sentence Redis provides high concurrency. seems redundant.

bessbd · 2017-10-29T12:29:20Z

flume-ng-doc/sphinx/FlumeUserGuide.rst

+
+.. code-block:: properties
+
+    a1.channels.channel1.type = org.flume.channels.redis.RedisChannel


We could add RedisChannel to flume/flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java so that people don't have to type org.flume.channels.redis.RedisChannel

bessbd · 2017-10-29T12:37:15Z

...channels/flume-redis-channel/src/main/java/org/apache/flume/channels/redis/RedisChannel.java

+      redisOperator = RedisInit.getInstance(redisConf, jedisPoolConfig);
+      counter.start();
+      super.start();
+    } catch (Exception e) {


I have noticed a few occurrences of catch (Exception e). According to https://stackoverflow.com/a/2416334/5323166 we should catch the most specific type we can. Can we catch more specific exceptions here?

bessbd · 2017-10-29T12:39:43Z

Overall, this change looks fine to me. After receiving feedback for the comments I have left, I'd like to request contributors and committers to do a review of this PR so that we can merge it.

change redis description

change channel type

channel exception type

JunLuo · 2017-11-11T03:35:23Z

Hi @bessbd ,
I've fix all problems you mentions in commits. :)

asfgit · 2018-08-17T13:09:14Z

Can one of the admins verify this patch?

pili-base -> pili

add redis channel

4cfc15d

add redis channel

JunLuo added 7 commits September 17, 2017 06:20

delete unused testapp

8cdbd76

delete unused testapp

add license comments

3eb654e

add license comments

fix codestyle check errors

6b2d8ca

fix codestyle check errors

fix codestyle check errors

c7d9eb7

fix codestyle check errors

fix codestyle check errors

558b1e4

fix codestyle check errors

init redis channel version

57832b3

init redis channel version

fix a stupid error

f48c013

fix a stupid error

fix a bug

401f7b3

fix a bug

add redis channel documentation

eff47db

add redis channel documentation

szaboferee reviewed Sep 26, 2017

View reviewed changes

JunLuo added 5 commits September 27, 2017 15:13

remove useness optional wrapper

9aa53c0

remove useness optional wrapper

do many changes

0b73407

`add sentinel and cluster type `move redis config to channel config `add unit test

fix code style

a130d7a

fix code style

fix code style error

f610bd5

fix code style error

add cluster and redis config documention

da40fa4

add cluster and redis config documention

bessbd reviewed Oct 29, 2017

View reviewed changes

JunLuo added 3 commits November 11, 2017 11:13

change redis description

7cd3755

change redis description

change channel type

7b56d8a

change channel type

channel exception type

034503c

channel exception type

waidr pushed a commit to waidr/flume that referenced this pull request Jul 24, 2019

Merge pull request apache#176 from lintianzhi/feature/pkg

2c2b6aa

pili-base -> pili


		.. code-block:: properties

		a1.channels.channel1.type = org.flume.channels.redis.RedisChannel

add redis channel #176

Are you sure you want to change the base?

add redis channel #176

Uh oh!

Conversation

JunLuo commented Sep 16, 2017

Uh oh!

bessbd commented Sep 16, 2017

Uh oh!

JunLuo commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bessbd commented Sep 25, 2017

Uh oh!

JunLuo commented Sep 26, 2017

Uh oh!

szaboferee left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

JunLuo commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JunLuo commented Oct 28, 2017

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!

bessbd commented Oct 29, 2017

Uh oh!

JunLuo commented Nov 11, 2017

Uh oh!

asfgit commented Aug 17, 2018

Uh oh!

Uh oh!

JunLuo commented Sep 16, 2017 •

edited

Loading

JunLuo commented Sep 29, 2017 •

edited

Loading