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

add redis implement of datasource #102

Merged
merged 7 commits into from
Sep 14, 2018

Conversation

tigerMoon
Copy link
Contributor

@tigerMoon tigerMoon commented Aug 28, 2018

Describe what this PR does / why we need it

Add Redis implementation of Datasource

Does this pull request fix one issue?

Resolves #37

Describe how you did it

Use Lettuce Redis client and Redis pub/sub feature.

Describe how to verify it

Run the test cases.

Special notes for reviews

no

@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

Merging #102 into master will increase coverage by 0.5%.
The diff coverage is 54.06%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #102     +/-   ##
===========================================
+ Coverage     45.74%   46.24%   +0.5%     
- Complexity      556      589     +33     
===========================================
  Files           114      118      +4     
  Lines          3817     4063    +246     
  Branches        531      559     +28     
===========================================
+ Hits           1746     1879    +133     
- Misses         1857     1950     +93     
- Partials        214      234     +20
Impacted Files Coverage Δ Complexity Δ
...csp/sentinel/datasource/redis/util/AssertUtil.java 33.33% <33.33%> (ø) 4 <4> (?)
...inel/datasource/redis/config/RedisHostAndPort.java 33.33% <33.33%> (ø) 6 <6> (?)
...csp/sentinel/datasource/redis/RedisDataSource.java 56.92% <56.92%> (ø) 6 <6> (?)
...datasource/redis/config/RedisConnectionConfig.java 58.45% <58.45%> (ø) 17 <17> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02cf5e5...fc4fbc4. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Aug 28, 2018
sentinel-extension/sentinel-datasource-redis/pom.xml Outdated Show resolved Hide resolved

private String ruleKey;

public RedisDataSource(RedisClient client, String ruleKey, String channel, ConfigParser<String, T> parser) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the constructor should not directly accept a RedisClient as users don't have to know the internal client here. Users can provide host and port, then we create the client internal. So even if we changed the Redis client library, users don't have to care about that.

The constructor can be like this:

public RedisDataSource(String host, int port, String ruleKey, String channel, ConfigParser<String, T> parser)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. it should be decoupled. but when build RedisClient with lettuce. it has different mode to build like this:

Standalone mode build:

RedisURI masterUri = RedisURI.Builder.redis("master-host", 6379).build();

Sentinel mode build:

RedisURI sentinelUri = RedisURI.Builder.sentinel("sentinel-host", 26379, "master-name").build();

case i build a own constructor to compatible to all:

MyRedisURI{host,port,password,RedisConnectionType,ClientResources}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, you can implement a universal RedisConfig class including these attributes, then users can pass a RedisConfig to the constructor.


@Override
public void message(String channel, String message) {
getProperty().updateValue(parser.parse(message));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a log here like this:

RecordLog.info(String.format("[RedisDataSource] New property value received for (%s:%d, %s): %s", host, port, channel, message));

@@ -0,0 +1,111 @@
package com.alibaba.csp.sentinel.datasource.redis;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add license header here.

}

@Test
public void pub_msg_and_receive_success() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name should follow camel-case naming style like: testPubMessageAndReceiveSuccess.

@sczyh30
Copy link
Member

sczyh30 commented Sep 3, 2018

Using pub-sub feature is a good idea, but for consistency users should publish the value and save the value to the ruleKey simultaneously like this (using Redis transaction):

MULTI
PUBLISH channel value
SET ruleKey value
EXEC

We cannot ensure users will do that, but it's better to mention it in the document, and provide a helper publisher to do that.

@sczyh30
Copy link
Member

sczyh30 commented Sep 6, 2018

Hi, any progress on this?

@tigerMoon
Copy link
Contributor Author

yes. I will finish it at this weekend.

@tigerMoon
Copy link
Contributor Author

Hi, any progress on this?

I'm done

@sczyh30
Copy link
Member

sczyh30 commented Sep 12, 2018

Nice! It may take some time to review the code. I'll do the review these days.

@sczyh30
Copy link
Member

sczyh30 commented Sep 14, 2018

Hi, please re-sign the CLA as there may be some update on it.

* @param masterId sentinel master id
* @return New builder with Sentinel host/port.
*/
public static RedisConnectionConfig.Builder sentinel(String host, int port, String masterId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to rename sentinel to redisSentinel as our project's name is also Sentinel, so the name of the method may be confusing.

@sczyh30
Copy link
Member

sczyh30 commented Sep 14, 2018

Thanks for your contribution! I'll rearrange some code later.

@sczyh30 sczyh30 merged commit 4ff2e37 into alibaba:master Sep 14, 2018
@sczyh30 sczyh30 removed the to-review To review label Sep 14, 2018
sczyh30 pushed a commit that referenced this pull request Sep 18, 2018
- This implementation uses Lettuce as the internal client, and leverages Redis pub-sub feature to implement push mode data source. (by @tigerMoon)
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019
- This implementation uses Lettuce as the internal client, and leverages Redis pub-sub feature to implement push mode data source. (by @tigerMoon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] DataSource integration for Redis
3 participants