-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[#3117]Just Sink the Notify implementation into common module and optimize some parts #3118
[#3117]Just Sink the Notify implementation into common module and optimize some parts #3118
Conversation
…ptimize some parts.
这个PR的几个改造要点: (1):将原来core module里面的NofityCenter和DefaultPublisher等下沉至common的module中,同时由于common module本身只是支持 java 6的,所以我把原来Nofity里面涉及Lamba表达式和Java 8的东西,都替换掉了(其中为了去除原本java 8的default关键字,我用了abstract抽象类来代替;将BiFunction从JDK 8中剥离移到common module中),没有做实际的重构和改造,基本是保留原来春少的Nofity设计思路的; (2):对于“System.currentTimeMillis()”设置sequence的方法,采用“AtomicInteger或者AtomicLong,减少对系统时间的查询” |
Please help to review this pr, @yanlinly @KomachiSion @chuntaojun |
common/src/main/java/com/alibaba/nacos/common/notify/DefaultPublisher.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/notify/Event.java
Outdated
Show resolved
Hide resolved
* @author <a href="mailto:[email protected]">liaochuntao</a> | ||
* @author zongtanghu | ||
*/ | ||
@SuppressWarnings("all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no special situation, please do not use SuppressWarnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will fix this in next commit.
common/src/main/java/com/alibaba/nacos/common/utils/BiFunction.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/utils/BiFunction.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/utils/ClassUtils.java
Outdated
Show resolved
Hide resolved
@Documented | ||
@Target({ElementType.TYPE, ElementType.METHOD}) | ||
@Retention(RetentionPolicy.SOURCE) | ||
public @interface NotThreadSafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you want to figure out some method may be not thread safe. But I want to know whether is project do like this? I think write some discription in Javadoc is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some open source project use this annotation to present unsafe method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@SuppressWarnings({"PMD.AbstractClassShouldStartWithAbstractNamingRule", "PMD.ConstantFieldShouldBeUpperCaseRule"}) | ||
public abstract class Event implements Serializable { | ||
|
||
private static final AtomicLong sequence = new AtomicLong(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that no-->sequence
the constant still is SEQUENCE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sorry , I disunderstand what's your meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this in next commit.
common/src/main/java/com/alibaba/nacos/common/notify/NotifyCenter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/notify/NotifyCenter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/notify/NotifyCenter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/notify/NotifyCenter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/utils/ClassUtils.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/utils/BiFunction.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/alibaba/nacos/common/notify/listener/Subscriber.java
Outdated
Show resolved
Hide resolved
@Documented | ||
@Target({ElementType.TYPE, ElementType.METHOD}) | ||
@Retention(RetentionPolicy.SOURCE) | ||
public @interface NotThreadSafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
} | ||
|
||
private boolean hasSubscriber() { | ||
return CollectionUtils.isNotEmpty(subscribers) || CollectionUtils.isNotEmpty(SMART_SUBSCRIBERS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being empty in the smartSubscribers collection does not imply that there are listeners interested in the event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this issue has already been resolved in this pr.
// If you want to listen to multiple events, you do it separately, | ||
// based on subclass's subscribeTypes method return list, it can register to publisher. | ||
if (consumer instanceof SmartSubscriber) { | ||
EventPublisher.SMART_SUBSCRIBERS.add((SmartSubscriber) consumer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete SMART_SUBSCRIBERS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this in next commit.
public static <T> void deregisterSubscriber(final Subscriber consumer) { | ||
final Class<? extends Event> cls = consumer.subscribeType(); | ||
if (consumer instanceof SmartSubscriber) { | ||
EventPublisher.SMART_SUBSCRIBERS.remove((SmartSubscriber) consumer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete SMART_SUBSCRIBERS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this in next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
后面建议把ut 也一起迁移下来。
Hi @yanlinly, 我再下一个pr里面把ut一起迁移到common库下面吧
[email protected]
From: yanlinly
Date: 2020-06-22 10:29
To: alibaba/nacos
CC: Hu Zongtang; Author
Subject: Re: [alibaba/nacos] [#3117]Just Sink the Notify implementation into common module and optimize some parts (#3118)
@yanlinly commented on this pull request.
后面建议把ut 也一起迁移下来。
―
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Sink the Notify implementation into common module and optimize some parts
Brief changelog
Sink the Notify implementation into common module and optimize some parts.
And this pr is one part of another bigger pr.The link is #2859.
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.