Mark, On Sun, Oct 6, 2024, 06:43 Mark Thomas <ma...@apache.org> wrote:
> On 06/10/2024 06:54, isa...@apache.org wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > isapir pushed a commit to branch main > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > > > The following commit(s) were added to refs/heads/main by this push: > > new 9cc44dcca2 Allow different implementations for RateLimitFilter > > 9cc44dcca2 is described below > > > > commit 9cc44dcca224ebc5fc4f8c835e3c64f42ab04fc1 > > Author: Igal Sapir <isa...@apache.org> > > AuthorDate: Sat Oct 5 22:54:21 2024 -0700 > > > > Allow different implementations for RateLimitFilter > > Overall, big +1 to this sort of extensibility. > > > + /** > > + * init-param to set a class name that implements RateLimiter > > + */ > > + public static final String PARAM_CLASS_NAME = "className"; > > I think this needs to be renamed for consistency. Generally, we use > className to define the implementing class of the component being > defined. For sub-components, we use something like rateLimiterClassName. > For an example, have a look at the Manager class where we have className > and secureRandomClass. > Sounds good. I was looking for example in the Connector configuration in server.xml and was unaware of the distinction between a component and a subcomponent. Are these rules/conventions documented anywhere? I'd be happy to start it if not but I'm not sure where would be a good place. Otherwise there is a risk that the next time I do something like that I will forget and use the wrong convention again. > > + try { > > + rateLimiter = > (RateLimiter)Class.forName(rateLimitClassName).getConstructor().newInstance(); > > + } catch (InstantiationException | IllegalAccessException | > InvocationTargetException | > > + NoSuchMethodException | ClassNotFoundException e) { > > You can catch ReflectiveOperationException here and simplify the above. > Ack > > diff --git a/test/org/apache/catalina/filters/TestRateLimitFilter.java > b/test/org/apache/catalina/filters/TestRateLimitFilter.java > > index 0d918285c7..ff9bdbf351 100644 > > --- a/test/org/apache/catalina/filters/TestRateLimitFilter.java > > +++ b/test/org/apache/catalina/filters/TestRateLimitFilter.java > > @@ -24,6 +24,7 @@ import jakarta.servlet.FilterChain; > > import jakarta.servlet.FilterConfig; > > import jakarta.servlet.ServletException; > > > > +import org.apache.catalina.util.FastRateLimiter; > > import org.junit.Assert; > > import org.junit.Test; > > > > @@ -55,9 +56,11 @@ public class TestRateLimitFilter extends > TomcatBaseTest { > > MockFilterChain filterChain = new MockFilterChain(); > > RateLimitFilter rateLimitFilter = > testRateLimitFilter(filterDef, root); > > > > - int allowedRequests = (int) > Math.round(rateLimitFilter.bucketCounter.getRatio() * bucketRequests); > > + FastRateLimiter tbc = (FastRateLimiter) > rateLimitFilter.rateLimiter; > > tbc? Looks like "To Be Confirmed". Maybe expand this name. > Understood and agreed > It is great you were able to address this so promptly. > Thank you. I will apply the changes above ASAP. Igal > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
