[ENHANCEMENT] Make the Apisix plugin reactive #1412
Conversation
| @@ -29,25 +31,32 @@ public String name() { | |||
| @Override | |||
| public void filter(HttpRequest request, HttpResponse response, PluginFilterChain chain) { | |||
| logger.debug("Received a new request"); | |||
| sidFilter(request, response); | |||
| chain.filter(request, response); | |||
| sidFilter(request, response) | |||
There was a problem hiding this comment.
It's possible that chain.filter(request, response) will be called AFTER (bad case) or BEFORE (perfect case) the "PluginFilterChain" had completed
+1 🍺 if it works
There was a problem hiding this comment.
It's callback based, it is fine!
That's Netty under the hood it feels like home.
There was a problem hiding this comment.
It's callback based, it is fine! That's Netty under the hood it feels like home.
Could you refer to any document that prove this "callback"? I agree with Tung. Code we put in reactor could be executed after response is sent to client.
There was a problem hiding this comment.
You are tiring me, so deeply, some days.
There was a problem hiding this comment.
Because implementing a callback interface:
-> it's clear that though asynchronous the callbacks are still executed in order
-> And sending the response to the client would trivially be implemented in the very last filter.
There was a problem hiding this comment.
apache/apisix-java-plugin-runner#313
Please qualify the new plugin with the new plugin runner ;-)
There was a problem hiding this comment.
Yeah with the updated plugin runner, the reactive code works well.
I have tested case token revoked --> pass
I have conducted performance test like this #1247 (comment) --> The result is good. The latency is always around 5s (I set timeout to 5s).
Regarding updating the plugin runner, I wonder if there is anyone reviewing and approving the PR. In case the repository is no longer maintained, I suggest we fork, create our own repository for the plugin runner (as well as CI/CD)
There was a problem hiding this comment.
Regarding updating the plugin runner, I wonder if there is anyone reviewing and approving the PR.
It is Christmass time.
Be patient.
We can package, build and deploy the PR for the time being.
Can we do a JMAP perf test on top of this change?
Can we publish the updated Apisix docker image with both changes to the runner and to the plugin?
I have conducted performance test like this #1247 (comment) --> The result is good. The latency is always around 5s (I set timeout to 5s).
(I told you: let's patch the causes and not the consequences)
There was a problem hiding this comment.
Apisix docker image: hungphan227/apisix:3.9.1-debian-javaplugin
There was a problem hiding this comment.
It is complicated to update staging env to run jmap perf test with apisix. Therefore, I will update james-gatling and run it locally with tmail-backend memory version.
| @@ -39,7 +39,7 @@ public class AppConfiguration { | |||
| @Value("${redis.password}") | |||
| private String redisPassword; | |||
|
|
|||
| @Value("${redis.timeout:5000}") | |||
| @Value("${redis.timeout:500}") | |||
|
Performance test result with 2 min inject, 8 min scenario, 1000 users, tmail memory (due to an unknown issue, I have to remove some JMAP requests in the test):
|
|
Ok thanks a lot @hungphan227 ! This proves:
Which image did you use to run those tests? |
Apisix docker image with new plugin: hungphan227/apisix:3.9.1-debian-javaplugin |
Build is not necessarily straight forward as it requires building the java plugin yourself. Maybe keep this as a draft until upstream is released? |
Please:
CF #1247
@vttranlina @hungphan227 please cary over this work from now on