[ISSUE #4750] Replace sun.net.httpserver.HttpServer to use netty server by karsonto · Pull Request #4739 · apache/eventmesh

karsonto · 2024-01-11T11:39:22Z

Fixes #4750 Replace sun.net.httpserver.HttpServer to use netty server

Motivation

Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

Describe the modifications you've done.

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Pil0tXia

Adding a AdminProcessor in EventMeshHTTPServer like AdminMetricsProcessor, is enough, admin is not a protocol.

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractRemotingServer.java

...time/src/main/java/org/apache/eventmesh/runtime/admin/controller/ClientManageController.java

...e/src/main/java/org/apache/eventmesh/runtime/admin/controller/HttpHandlerManagerAdapter.java

Pil0tXia · 2024-01-14T19:04:04Z

I didn't see changes under org.apache.eventmesh.runtime.admin.handler packages. They are using com.sun.net.httpserver.HttpExchange and can be replaced with netty usages.

Furthermore, com.sun.net.httpserver.HttpExchange also introduced complexity in logging exceptions. There is no need to print stacktrace twice and they can be replaced with a more concise usage like log.error("Runtime Admin endpoint failed to create topic: {}", e.getMessage());.

eventmesh/eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/TopicHandler.java

Lines 150 to 162 in e837f34

    
               httpExchange.sendResponseHeaders(200, 0); 
        
           } catch (Exception e) { 
        
               StringWriter writer = new StringWriter(); 
        
               PrintWriter printWriter = new PrintWriter(writer); 
        
               e.printStackTrace(printWriter); 
        
               printWriter.flush(); 
        
               String stackTrace = writer.toString(); 
        
               Error error = new Error(e.toString(), stackTrace); 
        
               String result = JsonUtils.toJSONString(error); 
        
               httpExchange.sendResponseHeaders(500, Objects.requireNonNull(result).getBytes(Constants.DEFAULT_CHARSET).length); 
        
               log.error(result, e); 
        
           }

karsonto · 2024-01-16T01:01:16Z

@Pil0tXia I think this issue should be handled in two PRs. 1. Convert the existing HTTP server to Netty server, and maintain the compatibility of the original Handler. 2. Change the HTTP handler to the Netty HttpRequestProcessor used in the project.

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminServer.java

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractRemotingServer.java

Pil0tXia · 2024-01-17T07:44:54Z

You may merge lastest master branch to fix CI errors.

Pil0tXia · 2024-01-17T10:42:47Z

I think this issue should be handled in two PRs. 1. Convert the existing HTTP server to Netty server, and maintain the compatibility of the original Handler. 2. Change the HTTP handler to the Netty HttpRequestProcessor used in the project.

@karsonto Please create two subtask issues of #4738 and link this PR to one of them.

Pil0tXia · 2024-01-17T14:40:01Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminServer.java

+        thread.start();
+        started.compareAndSet(false, true);
+    }


L86, better not to edit the startup status boolean of an abstract class (AbstractHTTPServer).

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractHTTPServer.java

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractRemotingServer.java

...-runtime/src/main/java/org/apache/eventmesh/runtime/admin/controller/HttpHandlerManager.java

Pil0tXia · 2024-01-17T14:59:12Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractHTTPServer.java

    @Sharable
-    private class HttpConnectionHandler extends ChannelDuplexHandler {
+    protected class HttpConnectionHandler extends ChannelDuplexHandler {



Is there any way to keep it private here?

EventMeshAdminServer can not use If this HttpConnectionHandler keep private .

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/util/HttpResponseUtils.java

Pil0tXia · 2024-01-18T10:16:40Z

...-runtime/src/main/java/org/apache/eventmesh/runtime/admin/controller/HttpHandlerManager.java

+    public void exec(ChannelHandlerContext ctx, HttpRequest httpRequest) {
+        String uriStr = httpRequest.uri();
+        URI uri = URI.create(uriStr);
+        HttpHandler httpHandler = httpHandlerMap.get(uri.getPath());
+        if (httpHandler != null) {
+            try {
+                HttpHandlerManager.AdminHttpExchange adminHttpExchange = new HttpHandlerManager.AdminHttpExchange(ctx, httpRequest);
+                httpHandler.handle(adminHttpExchange);
+                adminHttpExchange.writeAndFlash();
+                return;
+            } catch (Exception e) {
+                log.error(e.getMessage(), e);
+                ctx.writeAndFlush(HttpResponseUtils.createInternalServerError()).addListener(ChannelFutureListener.CLOSE);
+            }
+        } else {
+            ctx.writeAndFlush(HttpResponseUtils.createNotFound()).addListener(ChannelFutureListener.CLOSE);
+        }


How about integrating EventMeshAdminServer with HandlerService instead of implementing a group of http processing logic seperately for admin?

Pil0tXia

It seems that you haven't implement EventMeshAdminBootstrap.

karsonto · 2024-01-31T03:36:31Z

It seems that you haven't implement EventMeshAdminBootstrap.

Your means add AdminBootstrap startup list in EventMeshServer?

Pil0tXia · 2024-01-31T06:42:06Z

...time/src/main/java/org/apache/eventmesh/runtime/admin/controller/ClientManageController.java

 import org.apache.eventmesh.runtime.admin.handler.UpdateWebHookConfigHandler;
-import org.apache.eventmesh.runtime.boot.EventMeshAdminServer;
+import org.apache.eventmesh.runtime.boot.EventMeshAdminBootstrap;
 import org.apache.eventmesh.runtime.boot.EventMeshGrpcServer;


EventMeshAdminBootstrap should be referenced by EventMeshServer.

Got it,please review again

Pil0tXia · 2024-01-31T14:58:44Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshServer.java


-    private transient ClientManageController clientManageController;
+    //  private transient ClientManageController clientManageController;



Why commented here?

Pil0tXia · 2024-01-31T15:02:35Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminBootstrap.java


-import org.apache.eventmesh.runtime.admin.controller.HttpHandlerManager;
+import org.apache.eventmesh.runtime.admin.controller.ClientManageController;
 import org.apache.eventmesh.runtime.configuration.EventMeshHTTPConfiguration;


I feel that referencing ClientManageController in EventMeshAdminBootstrap is not a good practice. I think the design of ClientManageController and HttpHandlerManager has been outdated and it will be great if you may redesign them to fit EventMesh style of Netty HTTP Server.

That's right,please review it.

Pil0tXia · 2024-02-01T08:38:27Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminServer.java

+    private void registerHttpHandler() {
+        initHandler(new ShowClientHandler(eventMeshTCPServer));
+        initHandler(new ShowClientBySystemHandler(eventMeshTCPServer));
+        initHandler(new RejectAllClientHandler(eventMeshTCPServer));
+        initHandler(new RejectClientByIpPortHandler(eventMeshTCPServer));
+        initHandler(new RejectClientBySubSystemHandler(eventMeshTCPServer));
+        initHandler(new RedirectClientBySubSystemHandler(eventMeshTCPServer));
+        initHandler(new RedirectClientByPathHandler(eventMeshTCPServer));
+        initHandler(new RedirectClientByIpPortHandler(eventMeshTCPServer));
+        initHandler(new ShowListenClientByTopicHandler(eventMeshTCPServer));
+        initHandler(new QueryRecommendEventMeshHandler(eventMeshTCPServer));
+        initHandler(new TCPClientHandler(eventMeshTCPServer));
+        initHandler(new HTTPClientHandler(eventMeshHTTPServer));
+        initHandler(new GrpcClientHandler(eventMeshGrpcServer));
+        initHandler(new ConfigurationHandler(
+            eventMeshTCPServer.getEventMeshTCPConfiguration(),
+            eventMeshHTTPServer.getEventMeshHttpConfiguration(),
+            eventMeshGrpcServer.getEventMeshGrpcConfiguration()));
+        initHandler(new MetricsHandler(eventMeshHTTPServer, eventMeshTCPServer));
+        initHandler(new TopicHandler(eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshStoragePluginType()));
+        initHandler(new EventHandler(eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshStoragePluginType()));
+        initHandler(new MetaHandler(eventMeshMetaStorage));
+        if (Objects.nonNull(adminWebHookConfigOperationManage.getWebHookConfigOperation())) {
+            WebHookConfigOperation webHookConfigOperation = adminWebHookConfigOperationManage.getWebHookConfigOperation();
+            initHandler(new InsertWebHookConfigHandler(webHookConfigOperation));
+            initHandler(new UpdateWebHookConfigHandler(webHookConfigOperation));
+            initHandler(new DeleteWebHookConfigHandler(webHookConfigOperation));
+            initHandler(new QueryWebHookConfigByIdHandler(webHookConfigOperation));
+            initHandler(new QueryWebHookConfigByManufacturerHandler(webHookConfigOperation));
+        }
+    }


How about putting this method (registering endpoints) in a Manager class just like the ConsumerManager does in EventMeshHTTPServer?

Sure,please review.

Pil0tXia

LGTM. Please have a throughout test on endpoint handlers.

Need more reviewers to check whether this PR's AdminServer implementation fit the EventMesh style.

xwm1992 · 2024-02-02T02:10:04Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/EventHandler.java

-        String connectorPluginType,
-        HttpHandlerManager httpHandlerManager) {
-        super(httpHandlerManager);
+        String connectorPluginType) {


this param seems like storagePluginType, as well as the comment on this Class, not the connector plugin type.

This will be optimized in the future.Thank you.

pandaapo · 2024-03-21T11:00:52Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminServer.java

+                } catch (Exception ex) {
+                    log.error("AdminHttpServer shutdown error!", ex);
+                }
+                System.exit(-1);


Is it appropriate for the admin server to exit when it starts a failed process? Older versions of admin server didn't have such strong constraints. Please keep an eye on the community, I'm not sure if it's appropriate or not.

admin server启动失败进程就退出是否合适？旧版的admin server没有这么强的约束。请社区留意，我不知道合适与否。

…y server (apache#4739) * fix 4738 * fix some bug * fix bug * remove initProducerManager from AbstractRemotingServer init * bug fix * bug fix * some enhance * some enhance * add admin bootstrap * some enhance * remove HttpHandlerManager and ClientManageController. * modify some unit test * add admin http handlermanager

Pil0tXia reviewed Jan 11, 2024

View reviewed changes

Pil0tXia reviewed Jan 13, 2024

View reviewed changes

Pil0tXia requested changes Jan 17, 2024

View reviewed changes

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshAdminServer.java Show resolved Hide resolved

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/AbstractRemotingServer.java Outdated Show resolved Hide resolved

karsonto added 2 commits January 17, 2024 15:51

fix 4738

e6e013e

fix some bug

18708df

karsonto force-pushed the develop-#4738 branch from 4299a27 to 18708df Compare January 17, 2024 07:52

fix bug

ba86180

remove initProducerManager from AbstractRemotingServer init

ff7aaf5

Pil0tXia reviewed Jan 17, 2024

View reviewed changes

karsonto changed the title ~~[ISSUE #4738] Integrate Runtime admin endpoints with Netty server~~ [ISSUE #4750] Integrate Runtime admin endpoints with Netty server Jan 18, 2024

karsonto changed the title ~~[ISSUE #4750] Integrate Runtime admin endpoints with Netty server~~ [ISSUE #4750] Replace sun.net.httpserver.HttpServer to use netty server Jan 18, 2024

bug fix

3b74f5b

Pil0tXia requested changes Jan 18, 2024

View reviewed changes

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/util/HttpResponseUtils.java Outdated Show resolved Hide resolved

bug fix

b6a1ad4

Pil0tXia reviewed Jan 18, 2024

View reviewed changes

karsonto added 2 commits January 29, 2024 14:32

some enhance

b7a8247

some enhance

0e781ff

Pil0tXia reviewed Jan 30, 2024

View reviewed changes

add admin bootstrap

7d11718

Pil0tXia reviewed Jan 31, 2024

View reviewed changes

some enhance

0387daf

Pil0tXia reviewed Jan 31, 2024

View reviewed changes

karsonto added 2 commits February 1, 2024 15:24

remove HttpHandlerManager and ClientManageController.

6f698bc

modify some unit test

5d70e4c

Pil0tXia reviewed Feb 1, 2024

View reviewed changes

add admin http handlermanager

7b36bdb

Pil0tXia approved these changes Feb 1, 2024

View reviewed changes

xwm1992 reviewed Feb 2, 2024

View reviewed changes

xwm1992 approved these changes Feb 4, 2024

View reviewed changes

xwm1992 merged commit 648f3e9 into apache:master Feb 4, 2024

pandaapo reviewed Mar 21, 2024

View reviewed changes

Pil0tXia mentioned this pull request Apr 3, 2024

[Enhancement] Integrate Runtime admin endpoints with Netty server #4738

Closed

3 tasks

xwm1992 added this to the 1.11.0 milestone Dec 18, 2024


		private transient ClientManageController clientManageController;
		// private transient ClientManageController clientManageController;

Conversation

karsonto commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

Pil0tXia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pil0tXia commented Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karsonto commented Jan 16, 2024

Uh oh!

Uh oh!

Uh oh!

Pil0tXia commented Jan 17, 2024

Uh oh!

Pil0tXia commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pil0tXia Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pil0tXia left a comment

Choose a reason for hiding this comment

Uh oh!

karsonto commented Jan 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pil0tXia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karsonto commented Jan 11, 2024 •

edited

Loading

Pil0tXia commented Jan 14, 2024 •

edited

Loading

Pil0tXia Jan 18, 2024 •

edited

Loading

Pil0tXia left a comment •

edited

Loading