Aias00 commented on code in PR #7082:
URL: https://github.com/apache/shenyu/pull/7082#discussion_r4071938799
##########
shenyu-client/shenyu-client-core/src/main/java/org/apache/shenyu/client/core/disruptor/ShenyuClientRegisterEventPublisher.java:
##########
@@ -50,14 +52,49 @@ public static ShenyuClientRegisterEventPublisher
getInstance() {
*
* @param shenyuClientRegisterRepository shenyuClientRegisterRepository
*/
- public void start(final ShenyuClientRegisterRepository
shenyuClientRegisterRepository) {
- RegisterClientExecutorFactory factory = new
RegisterClientExecutorFactory();
+ public synchronized void start(final ShenyuClientRegisterRepository
shenyuClientRegisterRepository) {
+ if (Objects.nonNull(providerManage)) {
+ return;
+ }
+ RegisterClientExecutorFactory<DataTypeParent> factory = new
RegisterClientExecutorFactory<>();
factory.addSubscribers(new
ShenyuClientMetadataExecutorSubscriber(shenyuClientRegisterRepository));
- factory.addSubscribers(new
ShenyuClientURIExecutorSubscriber(shenyuClientRegisterRepository));
+ ShenyuClientURIExecutorSubscriber uriSubscriber =
createUriSubscriber(shenyuClientRegisterRepository);
+ factory.addSubscribers(uriSubscriber);
factory.addSubscribers(new
ShenyuClientApiDocExecutorSubscriber(shenyuClientRegisterRepository));
factory.addSubscribers(new
ShenyuClientMcpExecutorSubscriber(shenyuClientRegisterRepository));
- providerManage = new DisruptorProviderManage<>(factory);
- providerManage.startup();
+ DisruptorProviderManage<DataTypeParent> manage =
createProviderManage(factory);
+ try {
+ manage.startup();
+ uriSubscriber.start();
+ providerManage = manage;
+ } catch (RuntimeException ex) {
+ uriSubscriber.shutdown();
+ DisruptorProvider<DataTypeParent> provider = manage.getProvider();
+ if (Objects.nonNull(provider)) {
+ provider.shutdown();
+ }
+ throw ex;
+ }
+ }
+
+ /**
+ * Create URI subscriber.
+ *
+ * @param repository register repository
+ * @return URI subscriber
+ */
+ protected ShenyuClientURIExecutorSubscriber createUriSubscriber(final
ShenyuClientRegisterRepository repository) {
Review Comment:
Non-blocking design nit: these two `protected` factories exist only so
`ShenyuClientRegisterEventPublisherTest` can inject mocks. They widen the API
surface of a public singleton (any subclass anywhere can now override disruptor
construction).
Options if you want to keep production API tight:
- make them package-private and put the test in the same package
(`org.apache.shenyu.client.core.disruptor`), or
- drop them and use `Mockito.mockConstruction(DisruptorProviderManage.class,
...)` in the test.
Not a blocker — the behaviour is correct either way.
##########
shenyu-client/shenyu-client-core/src/main/java/org/apache/shenyu/client/core/disruptor/ShenyuClientRegisterEventPublisher.java:
##########
@@ -50,14 +52,49 @@ public static ShenyuClientRegisterEventPublisher
getInstance() {
*
* @param shenyuClientRegisterRepository shenyuClientRegisterRepository
*/
- public void start(final ShenyuClientRegisterRepository
shenyuClientRegisterRepository) {
- RegisterClientExecutorFactory factory = new
RegisterClientExecutorFactory();
+ public synchronized void start(final ShenyuClientRegisterRepository
shenyuClientRegisterRepository) {
+ if (Objects.nonNull(providerManage)) {
+ return;
+ }
+ RegisterClientExecutorFactory<DataTypeParent> factory = new
RegisterClientExecutorFactory<>();
factory.addSubscribers(new
ShenyuClientMetadataExecutorSubscriber(shenyuClientRegisterRepository));
- factory.addSubscribers(new
ShenyuClientURIExecutorSubscriber(shenyuClientRegisterRepository));
+ ShenyuClientURIExecutorSubscriber uriSubscriber =
createUriSubscriber(shenyuClientRegisterRepository);
+ factory.addSubscribers(uriSubscriber);
factory.addSubscribers(new
ShenyuClientApiDocExecutorSubscriber(shenyuClientRegisterRepository));
factory.addSubscribers(new
ShenyuClientMcpExecutorSubscriber(shenyuClientRegisterRepository));
- providerManage = new DisruptorProviderManage<>(factory);
- providerManage.startup();
+ DisruptorProviderManage<DataTypeParent> manage =
createProviderManage(factory);
+ try {
+ manage.startup();
+ uriSubscriber.start();
+ providerManage = manage;
Review Comment:
Good — assigning `providerManage` only after both `startup()` and
`uriSubscriber.start()` succeed is what makes the failure path retryable (the
field stays null, so the next `start()` builds a fresh manager).
One thing worth a line of Javadoc: once this field is set, a later `start()`
with a *different* `ShenyuClientRegisterRepository` is silently ignored. That
is the intended idempotency, but it is surprising to a reader who passes a
repository and sees it discarded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
