asafm commented on code in PR #21667:
URL: https://github.com/apache/pulsar/pull/21667#discussion_r1415746083
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -387,7 +417,7 @@ private String makeHttpRequest(boolean useTls, boolean
useAuth) throws Exception
}
private void setupEnv(boolean enableFilter, boolean enableTls, boolean
enableAuth, boolean allowInsecure,
- double rateLimit, boolean disableTrace) throws
Exception {
+ double rateLimit, boolean disableTrace, boolean
enableCompressionSupport) throws Exception {
Review Comment:
I know you just added a flag, but honestly, how can someone reading the
setupEnv(...`false`) really know you enabled compressionSupport?
I would either:
1. Write a `EnvBuilder` , that has `enableCompression()`
2. Add /* enableCompression= */ to help people who read this code on the
lines calling it.
BTW: Builder would not force you to change so many lines when adding a
method, since they all have defaults.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -266,6 +267,12 @@ public void addServlet(String path, ServletHolder
servletHolder, boolean require
attributeMap.forEach(context::setAttribute);
}
filterInitializer.addFilters(context, requiresAuthentication);
+
+ if (pulsar.getConfiguration().isHttpResponseCompressionEnabled()) {
+ GzipHandler gzipHandler = new GzipHandler();
+ context.setHandler(gzipHandler);
Review Comment:
They way I understand Jetty architecture, this shouldn't work. If you
continue your test below and actually verify you have a minimum number of
lines, and it has at least one metric you know for sure exists there, you will
parse it and make sure it works - well, this test should fail, unless I don't
understand Jetty.
Jetty works with handler nesting. You create a ServletContextHandler, and
you wrap it with a GZipHandler.
If you call `gzipHandler.setHandler(servletHandler)`, it means you wrapped
the servelet handler with GZip. Now you can add gzipHandler to the `handlers`
list.
Here you seem to do the other way around. ServletHandler is wrapping
gzipHandler.
Not sure if it should work.
So I would say: Please expand you test to make sure the context you get, can
be uncompressed using GZip and it actually contain a valid expected Prometheus
response.
Also, let's rename `context` to `servletHandler or `servletContextHandler`
to know it's a handler. Context is confusing name.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
