You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/06/10 10:14:08 UTC
[GitHub] [rocketmq] lizhanhui opened a new pull request, #4446: Revamp interceptor filter of RemotingCommand
lizhanhui opened a new pull request, #4446:
URL: https://github.com/apache/rocketmq/pull/4446
## What is the purpose of the change
Fix [issue 4437](https://github.com/apache/rocketmq/issues/4437)
## Brief changelog
Revamp RPC Hook design and implementation
## Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily. Notice, `it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR`.
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
- [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test` to make sure integration-test pass.
- [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] duhenglucky commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
duhenglucky commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153608848
> adding
> @dongeforever This pull intends to keep RPCHooks as it is. Third-party implementations will continue to work as expected. Internally, RPCHooks will be wrapped to Handler, which will have better-defined APIs and deliver neat and clear semantics.
>
> Further, we hope new plugins are developed on top of the new interface, bringing them fewer doubts during their development at error handling, logic control, etc.
>
> In addition, this PR solves other closely related defects: on the server-side, post-hooks will get executed when ctx.writeAndFlush is used, as is also pointed out by @duhenglucky on the client-side, post-hooks are executed for the async code path;
A very good change, not only optimizes RPCHook, but also solves the problem that the post method is not called in asynchronous mode, but compatibility is still the biggest bottom line, many developers will implement their own Hook in the process of adapting to their own business .
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1154611039
> BTW, for most users, it does not need the HanderContext, just use the RPCHook as usual. The flow control is usually used on the Server-Side, which wants to provide advanced features.
gRPC does split this feature into two interfaces, [ClientInterceptor](https://grpc.github.io/grpc-java/javadoc/io/grpc/ClientInterceptor.html) and [ServerInterceptor](https://grpc.github.io/grpc-java/javadoc/index.html?io/grpc/ServerInterceptor.html)
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] coveralls commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1152264573
[](https://coveralls.io/builds/49918771)
Coverage decreased (-0.2%) to 52.0% when pulling **d51213a76b6ad767d6a1611a9c99347dc0c3581a on lizhanhui:revamp_interceptor_chain** into **73b9ac82bcd14b2a40ba31888a96e62d06d42d92 on apache:develop**.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lwclover commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lwclover commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153718672
Implementation of pipeline by Handlers, each handler call a RPCHook.
In RPCHook, there may be deserialization and serialization, which is a time-consuming behavior, I don't think this is an appropriate way to work on it.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153615907
>
Both the two PRs don't break the compatibility. The discussion point is that should we introduce a new method like `registerHandler` or not.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153547387
@lizhanhui Most part of this PR is great.
The only doubt is that there is no need to introduce a new Handler to do this, adding context to the RPCHook works well.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1158440045
@lizhanhui
1. In the beginning, without the InterceptorAdaptor, the compatibility problem is serious. Currently, it is not so serious. But two Interfaces(RPCHook vs Interceptor) do a similar thing, which is not so good. And the RPCHook is marked deprecated, the end-user needs to change their code sooner or later, which will cause anxiety to the old user. This is not a good experience.
2. It is important to explain how often the flow control is used. In practice, the flow control is only used on the server-side, and only be used by the advanced developers(usually the service provider), Which means it is used by a very small number of developers. For the great majority, they do not need the flow control, especially on the client-side. So we'd better protect the end-user experience, which is the great majority.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1154667246
@lizhanhui I notice that you want to improve the developer experience.
But compatiblity is the most important experience for developers.
Envolve the RPCHook could continue the user's habit. The only time to break the old habit is that the old habit could not solve the problem any more.
But currently, it is easy to envolve the RPCHook to get the flow-control ability. So there is no need to add a new one to build a new habit?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895282198
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +178,55 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable ignore) {
+ }
+
+ if (Decision.STOP == decision) {
+ break;
+ }
+ }
+ return decision;
+ }
+
+
+ @Deprecated
protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
+ for (RPCHook rpcHook : rpcHooks) {
rpcHook.doBeforeRequest(addr, request);
}
}
}
+ @Deprecated
protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
+ for (RPCHook rpcHook : rpcHooks) {
Review Comment:
The code style inconsistency is fixed.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895306711
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -200,43 +245,79 @@ public void processRequestCommand(final ChannelHandlerContext ctx, final Remotin
public void run() {
try {
String remoteAddr = RemotingHelper.parseChannelRemoteAddr(ctx.channel());
+ final HandlerContext handlerContext = new HandlerContextAdaptor();
+ final CompletableFuture<RemotingCommand> responseFuture = new CompletableFuture<>();
+ if (Decision.STOP == NettyRemotingAbstract.this.preHandle(handlerContext, cmd, responseFuture)) {
+ if (cmd.isOnewayRPC()) {
+ return;
+ }
+ // Write response back to clients, which normally explains why the associated request
+ // fails handler chain.
+ RemotingCommand response;
+ if (responseFuture.isDone()) {
+ response = responseFuture.get();
+ } else {
+ final String message = "A handle is incorrectly implemented. " +
+ "It stopped the handler chain without setting ResponseFuture";
+ log.warn(message);
+ response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ message);
+ }
+ response.setOpaque(opaque);
+ ctx.writeAndFlush(response);
+ return;
+ }
+ // TODO: Remove the following line when RPC Hook reaches end of life.
doBeforeRpcHooks(remoteAddr, cmd);
Review Comment:
Done
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153603568
>
Hi @dongeforever, follow the PR https://github.com/apache/rocketmq/pull/4454, it seems that if we want to use the new hook behavior we must override the `AbstractRPCHook` and register it, right? If so, although we don't expose a new method, a new abstract class is exposed.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153551237
@dongeforever Let's wait to see more feedback from other community developers
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153647979
@zhouxinyu
The AbstractRPCHook is a compromised solution for JDK 1.6.
BTW, for most users, it does not need the HanderContext, just use the RPCHook as usual. The flow control is usually used on the Server-Side, which wants to provide advanced features.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895368602
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +173,50 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
- protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doBeforeRequest(addr, request);
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable e) {
+ RemotingCommand response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ e.getMessage());
+ response.setOpaque(request.getOpaque());
+ responseFuture.complete(response);
+ decision = Decision.STOP;
+ }
+
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
- protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doAfterResponse(addr, request, response);
+ protected Decision postHandle(final HandlerContext context, final RemotingCommand request, final RemotingCommand response) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.postHandle(context, request, response);
+ } catch (Throwable ignore) {
+ }
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
+ public void registerRPCHook(RPCHook rpcHook) {
+ if (null != rpcHook) {
+ handlers.add(new HandlerAdaptor(rpcHook));
+ }
Review Comment:
If the RPCHook is not exposed to the end-user, this Adapter is great.
But currently, the widely used DefaultMQProduer or DefaultMQConsumer, or DefaultAdminExt have the access to RPCHook.
It is better to come up with other way to solve it.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +173,50 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
- protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doBeforeRequest(addr, request);
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable e) {
+ RemotingCommand response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ e.getMessage());
+ response.setOpaque(request.getOpaque());
+ responseFuture.complete(response);
+ decision = Decision.STOP;
+ }
+
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
- protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doAfterResponse(addr, request, response);
+ protected Decision postHandle(final HandlerContext context, final RemotingCommand request, final RemotingCommand response) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.postHandle(context, request, response);
+ } catch (Throwable ignore) {
+ }
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
+ public void registerRPCHook(RPCHook rpcHook) {
+ if (null != rpcHook) {
+ handlers.add(new HandlerAdaptor(rpcHook));
+ }
Review Comment:
The flow-control is very useful, So why not apply the ability to the RPCHook, which could benefit the users without adding a new interface.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153539852
@dongeforever This pull intends to keep RPCHooks as it is. Third-party implementations will continue to work as expected.
Internally, RPCHooks will be wrapped to Handler, which will have better-defined APIs and deliver neat and clear semantics.
Further, we hope new plugins are developed on top of the new interface, bringing them fewer doubts during their development at error handling, logic control, etc.
In addition, this PR solves other closely related defects:
on the server-side, post-hooks will get executed when ctx.writeAndFlush is used, as is also pointed out by @duhenglucky
on the client-side, post-hooks are executed for the async code path;
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153522298
The RPCHook has been widely used for almost 10 years, and it has been exposed to end-users. The DefaultMQProducer and DefaultMQConsumer all have API to register RPCHook.
So we should not modify it unless we have to.
In fact, for flow control or any other demands, there is no need to modify the RPCHook. The PR https://github.com/apache/rocketmq/pull/4454 shows the code.
This PR needs a wide discussion before merging.
@lizhanhui @zhouxinyu @duhengforever
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1154616007
> Back to this PR, `registerHandler` is not a matched name compared to `RPCHook`. Perhaps, we could introduce a `EnhancedRPCHook` interface to make this feature easier to understand for both new and existing developers?
Maybe, we could rename / split Handler to ClientInterceptor / ServerInterceptor similar to gRPC.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r894431231
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +178,55 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable ignore) {
+ }
+
+ if (Decision.STOP == decision) {
+ break;
+ }
+ }
+ return decision;
+ }
+
+
+ @Deprecated
protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
+ for (RPCHook rpcHook : rpcHooks) {
rpcHook.doBeforeRequest(addr, request);
}
}
}
+ @Deprecated
protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
+ for (RPCHook rpcHook : rpcHooks) {
Review Comment:
Code style conflict
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui closed pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui closed pull request #4446: Revamp interceptor filter of RemotingCommand
URL: https://github.com/apache/rocketmq/pull/4446
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r894430839
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +178,55 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable ignore) {
Review Comment:
Exception means CONTINUE, is that reasonable?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895380766
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +173,50 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
- protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doBeforeRequest(addr, request);
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable e) {
+ RemotingCommand response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ e.getMessage());
+ response.setOpaque(request.getOpaque());
+ responseFuture.complete(response);
+ decision = Decision.STOP;
+ }
+
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
- protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doAfterResponse(addr, request, response);
+ protected Decision postHandle(final HandlerContext context, final RemotingCommand request, final RemotingCommand response) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.postHandle(context, request, response);
+ } catch (Throwable ignore) {
+ }
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
+ public void registerRPCHook(RPCHook rpcHook) {
+ if (null != rpcHook) {
+ handlers.add(new HandlerAdaptor(rpcHook));
+ }
Review Comment:
We have two goals: 1) Ensure original RPCHook implementations would continue to work; 2) Better development experience for further developers who develops plugins for RocketMQ
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895380766
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +173,50 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
- protected void doBeforeRpcHooks(String addr, RemotingCommand request) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doBeforeRequest(addr, request);
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable e) {
+ RemotingCommand response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ e.getMessage());
+ response.setOpaque(request.getOpaque());
+ responseFuture.complete(response);
+ decision = Decision.STOP;
+ }
+
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
- protected void doAfterRpcHooks(String addr, RemotingCommand request, RemotingCommand response) {
- if (rpcHooks.size() > 0) {
- for (RPCHook rpcHook: rpcHooks) {
- rpcHook.doAfterResponse(addr, request, response);
+ protected Decision postHandle(final HandlerContext context, final RemotingCommand request, final RemotingCommand response) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.postHandle(context, request, response);
+ } catch (Throwable ignore) {
+ }
+ if (Decision.STOP == decision) {
+ break;
}
}
+ return decision;
}
+ public void registerRPCHook(RPCHook rpcHook) {
+ if (null != rpcHook) {
+ handlers.add(new HandlerAdaptor(rpcHook));
+ }
Review Comment:
We have two goals: 1) Ensure original RPCHook implementations would continue to work; 2) Better development experience for further developers who develop plugins for RocketMQ
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r895280924
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +178,55 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable ignore) {
Review Comment:
After a second thought, let's change the decision to stop and generate response command according to the exception raised.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -166,22 +178,55 @@ public void processMessageReceived(ChannelHandlerContext ctx, RemotingCommand ms
}
}
+ protected Decision preHandle(final HandlerContext context, final RemotingCommand request,
+ final CompletableFuture<RemotingCommand> responseFuture) {
+ Decision decision = Decision.CONTINUE;
+ for (Handler handler : handlers) {
+ try {
+ decision = handler.preHandle(context, request, responseFuture);
+ } catch (Throwable ignore) {
Review Comment:
After a second thought, let's change the decision to stop and generate a response command according to the exception raised.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] duhenglucky commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
duhenglucky commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153669863
> >
>
> @duhenglucky Both the two PRs don't break the compatibility. The discussion point is that should we introduce a new method like `registerHandler` or not.
> >
>
> @duhenglucky Both the two PRs don't break the compatibility. The discussion point is that should we introduce a new method like `registerHandler` or not.
Sorry for didn't notice that this pr moved the registerRPCHook method to the Abstract class, However, for most of the exposed APIs or Hooks, the cost of migration should be avoided as much as possible. I prefer to continue to evolve on RPCHook. RPCHook and Handler seem to be two concepts， AbstractRPCHook provides a solution, but the current implementation, especially in JDK 1.6 doesn't seem to be friendly enough. In the long run, we will also upgrade JDK 1.6 support to 1.8 or higher, but this depends on the outcome of community discussions
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153650778
The AbstractRPCHook is like an advanced tool. In fact, the third-party project could provide enhanced implementation without requiring the user to change their application code.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on a diff in pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on code in PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#discussion_r894436028
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -200,43 +245,79 @@ public void processRequestCommand(final ChannelHandlerContext ctx, final Remotin
public void run() {
try {
String remoteAddr = RemotingHelper.parseChannelRemoteAddr(ctx.channel());
+ final HandlerContext handlerContext = new HandlerContextAdaptor();
+ final CompletableFuture<RemotingCommand> responseFuture = new CompletableFuture<>();
+ if (Decision.STOP == NettyRemotingAbstract.this.preHandle(handlerContext, cmd, responseFuture)) {
+ if (cmd.isOnewayRPC()) {
+ return;
+ }
+ // Write response back to clients, which normally explains why the associated request
+ // fails handler chain.
+ RemotingCommand response;
+ if (responseFuture.isDone()) {
+ response = responseFuture.get();
+ } else {
+ final String message = "A handle is incorrectly implemented. " +
+ "It stopped the handler chain without setting ResponseFuture";
+ log.warn(message);
+ response = RemotingCommand.createResponseCommand(RemotingSysResponseCode.SYSTEM_ERROR,
+ message);
+ }
+ response.setOpaque(opaque);
+ ctx.writeAndFlush(response);
+ return;
+ }
+ // TODO: Remove the following line when RPC Hook reaches end of life.
doBeforeRpcHooks(remoteAddr, cmd);
Review Comment:
How about wrapping registered RpcHooks with a default `Handler` implementation which regards exception as `STOP`?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1154611972
@lwclover
> Implementation of pipeline by Handlers, each handler call a RPCHook.
Yes, this is how this PR works.
> In RPCHook, there may be deserialization and serialization, which is a time-consuming behavior, I don't think this is an appropriate way to work on it.
No, there is no serialization or deserialization involved.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1154614892
@duhenglucky
> the cost of migration should be avoided as much as possible.
There is no cost or migration required. The existing code would continue to work as expected.
> I prefer to continue to evolve on RPCHook
There is no graceful way of evolving on RpcHook under current constraints. If the 'Default Method' feature of interface is possible, aka, relaxing JDK to 8+, we could as well add an extra pair preXXX and postXXX and delegate current ones to them.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] fuyou001 commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
fuyou001 commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153577718
the new feature is only suppport jdk7+,others looks good to me.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1153609374
Back to this PR, `registerHandler` is not a matched name compared to `RPCHook`. Perhaps, we could introduce a `EnhancedRPCHook` interface to make this feature easier to understand for both new and existing developers?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] lizhanhui commented on pull request #4446: Revamp interceptor filter of RemotingCommand
Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4446:
URL: https://github.com/apache/rocketmq/pull/4446#issuecomment-1157195869
@dongeforever
> But compatibility is the most important experience for developers.
This PR suffers no compatibility issue at all, which has been explicitly stated previously.
> it is easy to evolve the RPCHook to get the flow-control ability.
This is simply not true. Your proposing alternative suffers in multiple aspects
1. The proposing alternative forces developers to inherit AbstractRpcHook. Considering Java follows single inheritance, it might be a blocking issue for a few cases.
2. Even if AbstractRpcHook is used, users have to set Decision to HandlerContext. Compared to the return-value approach, this API is less intuitive and error-prone.
3. [Context](https://en.wikipedia.org/wiki/Context_(computing)) in most cases are used to feed info to tasks. It is rarely used to return a value, especially when it is not part of the function signature.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org