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

[Discuss] Shall we change Nacos-Client naming listener notify way (sync -> async) #4115

Closed
horizonzy opened this issue Nov 2, 2020 · 5 comments
Assignees

Comments

@horizonzy
Copy link
Collaborator

Describe the topc
Now in com.alibaba.nacos.client.naming.core.EventDispatcher.Notifier.run, when get serviceChange from server, it will notify com.alibaba.nacos.client.naming.core.EventDispatcher.Notifier.run, find listener by serviceKey, and execute the callback.
But the callback is sync to execute, if some long time operation in the callback, it will blocking other service's notify.
Shall we add some info in docs to hint user, this callback is sync, if do long time operation, it will influence other listener or service to notify. let user execute it by thread they defined by themselves.
Or execute callback in thread pool which realized by framework.

@chuntaojun
Copy link
Collaborator

You can use NotifyCenter instead, which is already in the common module

@horizonzy
Copy link
Collaborator Author

I mean this logic
image

@chuntaojun
Copy link
Collaborator

In the end, all event mechanisms need to be gathered into NotifyCenter. The logic you want has been implemented internally. You can see

@chuntaojun
Copy link
Collaborator

@Override
public void notifySubscriber(final Subscriber subscriber, final Event event) {
        
        LOGGER.debug("[NotifyCenter] the {} will received by {}", event, subscriber);
        
        final Runnable job = new Runnable() {
            @Override
            public void run() {
                subscriber.onEvent(event);
            }
        };
        
        final Executor executor = subscriber.executor();
        
        if (executor != null) {
            executor.execute(job);
        } else {
            try {
                job.run();
            } catch (Throwable e) {
                LOGGER.error("Event callback exception : {}", e);
            }
        }
}

@horizonzy
Copy link
Collaborator Author

yeah, 👍

chuntaojun pushed a commit that referenced this issue Nov 13, 2020
* refactor and supply api to async handle instances change event

* add async handle way to callback event listener

* refactor EventListener, supply getExecutor to handle event async and compatible old version.

* remove unnecessary code

* ignore abstract class name check

* fix EmptyLineSeparator

* remove unuseful import

* compatible old client sdk, add abstractEventListener to support async handle naming event.

* remove unuseful import

* refactor InstanceChangeListener. decouple AbstractEventListener's executor and InstanceChangeListener's executor

* 1.remove key if listeners is empty.
2.fix getSubscribeServices

* revert notify center code

* code quality enhance

* upgrade @SInCE 1.4.0 -> 1.4.1

* change combineListenKey to ServiceInfo.getKey

* just deregisterSubscriber when hostReactor shutdown

* not export getSubscribers()

* add this key word, and move code place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants