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

[ISSUE #8428] fix naming subscribe bug when multiple NamingService (2.x) #8433

Merged
merged 1 commit into from
May 27, 2022

Conversation

liqipeng
Copy link
Contributor

Detail : #8428

@KomachiSion
Copy link
Collaborator

  1. Please rebase develop, we has fixed the CI and IT problem.
  2. Should we use an UUID to identity single Client(NamingService), not for each subscriber.

@liqipeng
Copy link
Contributor Author

liqipeng commented May 23, 2022

  1. Please rebase develop, we has fixed the CI and IT problem.
  2. Should we use an UUID to identity single Client(NamingService), not for each subscriber.

@KomachiSion 我的实现是使用UUID来标识一个NamingService的,notifierEventScope就是这个标识:

    private String notifierEventScope;

    public NacosNamingService(String serverList) throws NacosException {
        Properties properties = new Properties();
        properties.setProperty(PropertyKeyConst.SERVER_ADDR, serverList);
	@@ -87,11 +90,12 @@ private void init(Properties properties) throws NacosException {
        InitUtils.initSerialization();
        InitUtils.initWebRootContext(properties);
        initLogName(properties);

        this.notifierEventScope = UUID.randomUUID().toString();    // 每个NacosNamingService生成一个UUID标识事件通知作用域
        this.changeNotifier = new InstancesChangeNotifier(this.notifierEventScope);
        NotifyCenter.registerToPublisher(InstancesChangeEvent.class, 16384);
        NotifyCenter.registerSubscriber(changeNotifier);
        this.serviceInfoHolder = new ServiceInfoHolder(namespace, this.notifierEventScope, properties);
        this.clientProxy = new NamingClientProxyDelegate(this.namespace, serviceInfoHolder, properties, changeNotifier);
    }

@liqipeng
Copy link
Contributor Author

liqipeng commented May 23, 2022

1、Event增加了一个scope(),默认实现返回null;Subscriber增加了一个 scopeMatches 的扩展方法,用于判断事件的作用域是否匹配

    /**
     * Whether the event's scope matches current subscriber. Default implementation is all scopes matched.
     * If you override this method, it better to override related {@link com.alibaba.nacos.common.notify.Event#scope()}.
     *
     * @param event {@link Event}
     * @return Whether the event's scope matches current subscriber
     */
    public boolean scopeMatches(T event) {
        return event.scope() == null;   // 默认判断==null,但是可以由子类覆默认实现
    }

2、DefaultPublisher

    void receiveEvent(Event event) {
        final long currentEventSequence = event.sequence();
        
        if (!hasSubscriber()) {
            LOGGER.warn("[NotifyCenter] the {} is lost, because there is no subscriber.", event);
            return;
        }

        // Notification single event listener
        for (Subscriber subscriber : subscribers) {
            if (!subscriber.scopeMatches(event)) { // 这里执行scopeMatches来进行判断
                continue;
            }

            // Whether to ignore expiration events
            if (subscriber.ignoreExpireEvent() && lastEventSequence > currentEventSequence) {
                LOGGER.debug("[NotifyCenter] the {} is unacceptable to this subscriber, because had expire",

3、InstancesChangeNotifier实现了自己的scopeMatches

    @Override
    public boolean scopeMatches(InstancesChangeEvent event) {
        return this.eventScope.equals(event.scope());
    }

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #8433 (49723de) into develop (7ca069d) will increase coverage by 0.06%.
The diff coverage is 85.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #8433      +/-   ##
=============================================
+ Coverage      41.13%   41.20%   +0.06%     
- Complexity      4379     4387       +8     
=============================================
  Files            873      873              
  Lines          31811    31825      +14     
  Branches        3706     3707       +1     
=============================================
+ Hits           13085    13113      +28     
+ Misses         17413    17398      -15     
- Partials        1313     1314       +1     
Impacted Files Coverage Δ
.../alibaba/nacos/common/notify/DefaultPublisher.java 65.47% <0.00%> (-1.60%) ⬇️
...ibaba/nacos/common/notify/listener/Subscriber.java 75.00% <0.00%> (-25.00%) ⬇️
...libaba/nacos/client/naming/NacosNamingService.java 87.83% <100.00%> (+0.08%) ⬆️
...a/nacos/client/naming/cache/ServiceInfoHolder.java 76.52% <100.00%> (+0.20%) ⬆️
...acos/client/naming/event/InstancesChangeEvent.java 100.00% <100.00%> (ø)
...s/client/naming/event/InstancesChangeNotifier.java 82.35% <100.00%> (+2.35%) ⬆️
...in/java/com/alibaba/nacos/common/notify/Event.java 100.00% <100.00%> (ø)
...alibaba/nacos/client/config/impl/ClientWorker.java 50.78% <0.00%> (-0.20%) ⬇️
.../core/v2/event/publisher/NamingEventPublisher.java 75.00% <0.00%> (+1.25%) ⬆️
... and 3 more

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 7ca069d...49723de. Read the comment docs.

@liqipeng liqipeng force-pushed the lqp/v2.x-develop_issue8428 branch from 1bcbdc8 to 49723de Compare May 23, 2022 18:51
@liqipeng liqipeng marked this pull request as ready for review May 24, 2022 00:58
@liqipeng
Copy link
Contributor Author

liqipeng commented May 24, 2022

@KomachiSion

  1. Please rebase develop, we has fixed the CI and IT problem.
  2. Should we use an UUID to identity single Client(NamingService), not for each subscriber.

CI、IT问题已解决;
上面评论增加了此PR变更的思路说明

@KomachiSion
Copy link
Collaborator

@KomachiSion

  1. Please rebase develop, we has fixed the CI and IT problem.
  2. Should we use an UUID to identity single Client(NamingService), not for each subscriber.

CI、IT问题已解决; 上面评论增加了此PR变更的思路说明

OK, I will review it again.

@hj-545
Copy link

hj-545 commented Dec 1, 2023

这个升级不是一个平滑的升级方案!
应该是使用按需实现的!我压根没有这样的需求,一升级client包(1.4.x --> 1.4.6),原来的订阅功能完全失效,而且也没有告警没有异常!

@liqipeng
Copy link
Contributor Author

liqipeng commented Feb 1, 2024

这个升级不是一个平滑的升级方案! 应该是使用按需实现的!我压根没有这样的需求,一升级client包(1.4.x --> 1.4.6),原来的订阅功能完全失效,而且也没有告警没有异常!

确实是没考虑到NotifyCenter.registerSubscriber这个api订阅全局InstancesChangeEvent的场景。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants