You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/03 10:22:09 UTC

[GitHub] [pulsar] lhotari opened a new pull request, #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

lhotari opened a new pull request, #15637:
URL: https://github.com/apache/pulsar/pull/15637

   ### Motivation
   
   Follow up on #14353
   
   There were filter instances for each context path which made `maxConcurrentHttpRequests` and `httpRequestsMaxPerSecond` configuration options not work as expected. The actual limits would be multiple times higher dependencing on the way clients access different context paths.
   
   ### Modifications
   
   - Refactor solution to use FilterHolder for sharing Servlet Filter instances across multiple servlet context paths
   - Fix invalid Jersey API usage
     -  `config.packages("jersey.config.server.provider.packages", javaPackages);` is invalid
       - The API takes a java package directly, passing `jersey.config.server.provider.packages` doesn't make any sense.
     - When configuring individual resource classes, `.register` method should be used instead of registering the package which includes the individual resource class.
     - Don't use recursive resource class scanning since it might results in unintended resource classes being 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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15637:
URL: https://github.com/apache/pulsar/pull/15637#discussion_r888633378


##########
pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/WorkerServer.java:
##########
@@ -171,18 +174,52 @@ private void init() {
         server.setConnectors(connectors.toArray(new ServerConnector[connectors.size()]));
     }
 
-    public static ServletContextHandler newServletContextHandler(String contextPath,
+    private static class FilterInitializer {

Review Comment:
   I see that this class is duplicated across several classes. What is the motivation for duplicating the 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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar] lhotari closed pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure
URL: https://github.com/apache/pulsar/pull/15637


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15637:
URL: https://github.com/apache/pulsar/pull/15637#discussion_r889475643


##########
pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/WorkerServer.java:
##########
@@ -171,18 +174,52 @@ private void init() {
         server.setConnectors(connectors.toArray(new ServerConnector[connectors.size()]));
     }
 
-    public static ServletContextHandler newServletContextHandler(String contextPath,
+    private static class FilterInitializer {

Review Comment:
   I agree with your assessment, thanks for sharing your motivation.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar] lhotari commented on a diff in pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15637:
URL: https://github.com/apache/pulsar/pull/15637#discussion_r888820094


##########
pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/WorkerServer.java:
##########
@@ -171,18 +174,52 @@ private void init() {
         server.setConnectors(connectors.toArray(new ServerConnector[connectors.size()]));
     }
 
-    public static ServletContextHandler newServletContextHandler(String contextPath,
+    private static class FilterInitializer {

Review Comment:
   > I see that this class is duplicated across several classes. What is the motivation for duplicating the code?
   
   I don't see a big difference whether the logic is in this new class compared to the previous duplication which is in methods. This PR didn't introduce the duplication which was in methods before moving it to these internal FilterInitializer classes. 
   
   I agree that it would be useful to refactor the solution to get rid of the duplication. I was thinking of that and also did a short spike, but the end result would have been a large refactoring. This would make it harder to understand what was changed in this particular case for getting rid of the duplicate servlet filter instances.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [pulsar] lhotari merged pull request #15637: [Broker][Proxy][Function worker] Remove duplicate servlet filter instances to fix backpressure

Posted by GitBox <gi...@apache.org>.
lhotari merged PR #15637:
URL: https://github.com/apache/pulsar/pull/15637


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org