Skip to content
This repository was archived by the owner on Oct 13, 2024. It is now read-only.

SCB-1374 support fsm redis channel#511

Closed
cmonkey wants to merge 45 commits intoapache:masterfrom
cmonkey:feature/1317
Closed

SCB-1374 support fsm redis channel#511
cmonkey wants to merge 45 commits intoapache:masterfrom
cmonkey:feature/1317

Conversation

@cmonkey
Copy link
Copy Markdown
Contributor

@cmonkey cmonkey commented Jul 23, 2019

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 23, 2019

Coverage Status

Coverage decreased (-1.7%) to 81.111% when pulling e5da8d0 on cmonkey:feature/1317 into fa608a2 on apache:master.

@@ -0,0 +1,85 @@
package org.apache.servicecomb.pack.alpha.fsm.channel.redis;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add the Apache License heard here

this.sendTo(event);
metricsService.metrics().doEventAccepted();
} catch (Exception ex) {
logger.error("send Exception = [{}]", ex);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

log error can show the stack trace if the last object is exception, but I'm not sure if it works for your case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which reminds me, here should throw an exception

@@ -0,0 +1,7 @@
package org.apache.servicecomb.pack.alpha.fsm.channel.redis;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

License header.

pom.xml Outdated
<netty.boringssl.version>2.0.7.Final</netty.boringssl.version>
<netty.version>4.1.24.Final</netty.version>
<zookeeper.version>3.4.13</zookeeper.version>
<fastjson.version>1.2.58</fastjson.version>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we add new dependency, we need to update the license file here

@WillemJiang WillemJiang self-requested a review July 24, 2019 00:32
Copy link
Copy Markdown
Member

@WillemJiang WillemJiang left a comment

Choose a reason for hiding this comment

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

Please add the JIRA number in your commit log.

@cmonkey cmonkey closed this Jul 24, 2019
@cmonkey cmonkey reopened this Jul 24, 2019
@Override
public void sendTo(BaseEvent event){
throw new UnsupportedOperationException("Doesn't implement yet!");
LOG.info("sendTo message = [{}]",event);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest using LOG.debug


@Override
public void publish(Object data) {
logger.info("send message [{}] to [{}]", data, channelTopic.getTopic());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest using

if(LOG.isDebugEnabled()){
  LOG.debug(xxx)
}

@coolbeevip
Copy link
Copy Markdown
Member

coolbeevip commented Jul 25, 2019

When Alpha cluster, only one RedisMessageSubscriber should be allowed at the same time. Otherwise, it will repeat consumption.

You can use the following method to determine whether the current node is the master node, Or you can implement a distributed lock based on Redis inside your component, only one Redis subscriber at a time

@Autowride
NodeStatus nodeStatus;

if(nodeStatus.isMaster()){
  // Is master
}

@cmonkey cmonkey closed this Jul 26, 2019
@cmonkey cmonkey reopened this Jul 26, 2019
@cmonkey cmonkey closed this Jul 27, 2019
@cmonkey cmonkey reopened this Jul 27, 2019
@cmonkey cmonkey closed this Jul 29, 2019
@cmonkey cmonkey deleted the feature/1317 branch July 29, 2019 03:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants