-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Test gzip support in RESTEasy Reactive #16924
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaumyaSingh1,
I suggested some small changes.
.addClasses(BeanRegisteringRouteUsingObserves.class)); | ||
|
||
@Test | ||
public void test() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method nomenclature is too generic.
If you can make it clear what is being tested
public class GZipTest { | ||
|
||
private static final String APP_PROPS = "" + | ||
"quarkus.http.enable-compression=true\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of a test with the other way?
quarkus.http.enable-compression=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is disabled, should we really perform any sort of test? I don't think that's needed when is not even enabled :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but how to ensure that the property is working correctly?
and how do you make sure that no future code from other developers doesn't affect this and go undetected because not have any tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroMage WDYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we should test the default behaviour, that's true, but because we know what the default is ATM, we want to test the default behaviour without any configuration, to make sure it won't be compressed by default.
|
||
public void register(@Observes Router router) { | ||
|
||
router.route("/compression").handler(rc -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing reactive routes, but we want to be testing RESTEasy Reactive :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From available articles (https://quarkus.io/guides/reactive-routes) I got the reactive route part, I tried searching for resources around testing RR but didn't get. Maybe we are supposed to do something like ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait, perhaps I should be testing this against some RR endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed.
.header("content-length", Matchers.not(Matchers.equalTo(Integer.toString(longString.length())))) | ||
.body(Matchers.equalTo(longString)); | ||
|
||
RestAssured.given().get("/nocompression").then().statusCode(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For good measure, we should probably also test that we can send compressed data to the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here we want to test if the data we are sending is compressed or not ( i.e Inputstream is gzipped or zipped ). Right?
Or do we want to compress the files first before sending them? (but there will be no purpose of enabling gzip then, so not this one)
...uarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/GZipTest.java
Outdated
Show resolved
Hide resolved
…ent/src/test/java/io/quarkus/resteasy/reactive/GZipTest.java Co-authored-by: Stéphane Épardaud <[email protected]>
Registering endpoints
...uarkus-resteasy-reactive/deployment/src/test/java/io/quarkus/resteasy/reactive/GZipTest.java
Outdated
Show resolved
Hide resolved
In order to automate the process of "stoping the compression" we want to create an annotation. Situations/conditions when we want to stop/disable compression -
|
.header("content-encoding", "identity") | ||
.header("content-length", Matchers.equalTo(Integer.toString(longString.length()))) | ||
.body(Matchers.equalTo(longString)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does this test pass? I don't see anything in the endpoints that would justify a different behaviour.
Could you test with returning a binary reponse, such as an image? Just to see what vert.x does in this case, if it stll tries to compress it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the beginning I am initializing APP_PROPS as true
private static final String APP_PROPS = "" +
"quarkus.http.enable-compression=true\n";
perhaps that's why content encoding is always gzip
even for Binary Data i.e Images
That means vert.x is trying to compress the binary data as well, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So perhaps the conclusion is - It is trying to compress images as well.
Am I right @FroMage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably, could you add that test to your PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure, I missed this comment.
Well, first I'd like to see you test what the current behaviour is: will binary types get compressed or not? |
Since we can't enable compression on a per-endpoint basis, I guess we really need to add a |
Interesting, so now I am supposed to create an annotation And we will add this annotation at the endpoint |
Yes. |
|
||
RestAssured.given().get("/test/nocompression").then().statusCode(200) | ||
.header("content-encoding", "identity") | ||
.header("content-length", Matchers.equalTo((long) (createImage().getData().getDataBuffer().getSize() * 4L))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(long) (createImage().getData().getDataBuffer().getSize() * 4L)
Finding the content-length
for BufferedImage
is something I am not much sure about.
I took reference from here -
https://stackoverflow.com/questions/632229/how-to-calculate-java-bufferedimage-filesize?noredirect=1&lq=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of this png image(original) is 2.91kb
OR 2910 bytes
Using the above technique, the size is 360000
bytes.
On calculating content-length
using ByteArrayOutputStream
I am getting 2914 bytes
. ✔️
So perhaps this is the correct techniques to get the byte size of BufferedImage :
BufferedImage image = createImage();
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
ImageIO.write(image, "png", outputStream);
outputStream.close();
long contentLength = outputStream.size();
System.out.println(" content-length using ByteArrayOutputStream => " + contentLength );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you can just use a static test image as a resource :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm I thought it's better to generate an image only when this test is run.
You are right, it will make code simpler. Instead of creating an image through code it is better to use a static image 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImageIO.write(bufferedImage, "png", file); | ||
return bufferedImage; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created the annotations, but I am yet to understand and implement the logic that how to make my custom annotation set the header Am I going in the right direction? @FroMage |
I think your annotation should live next to So if the endpoint has compression disabled, we need a new handler that adds the required header after the endpoint is invoked, probably before this line: addHandlers(handlers, method, info, HandlerChainCustomizer.Phase.AFTER_METHOD_INVOKE); BTW, I'll be on PTO the next three days so take your time, or ask others for help on Zulip if you need to, but it's possible they'll also be on PTO. |
Sure, it's public holiday here as well (tomorrow ig). I will use your explanation ^ and try to understand, implement it. |
In order to do this ^ I tried thinking of these 2 ways : 1. Should I directly introduce the new annotation inside Line 377 in c88b990
ResourceMethod Line 500 in c88b990
OR Line 50 in 2cbd177
createResourceMethod() and set the properties in a way to disable compression.
|
While working on the previous filter and other tasks I was easily able to debug my code written in How should I go about it here e.g to check breakpoints in |
I would say option 1, because this is not specific to Quarkus. |
Breaking on the indexer requires you to run the build steps, but you can do that by adding a |
Hi @saumya1singh, any chance you'll resume this soon? |
hey @gastaldi, after 2.2.5 I am focusing on the Elektra release (plus have to provide training sessions to new interns in prod) , so can't give much time on this issue :/ |
@saumya1singh no worries, just curious 😉 |
For the record - it seems that vertx will try to compress the static resources as well, see the |
Hm, to be honest I don't like the idea of opt-out via In theory, we could introduce two annotations ( |
@geoand @cescoffier could we try to come up with a plan so that @mkouba can put it into motion. As for the requirements, I think:
I don't think we need fancy algorithm to start with. Let's keep it simple and we will see later if we can expand. |
I'll have a look (although I know nothing about how Vert.x supports gzip), but honestly I don't conside this terribly important fot |
Sorry, I don't have much availability this week. First, the static handler can skip compression for a set of media types or extensions. I think we should rely on this. Basically, I would recommend enabling compression by default and letting decide compression vs. raw using the annotation proposed by @mkouba. I know it's a big change in terms of behavior, but I think it's reasonable, and the user can go back to the previous behavior by disabling the compression globally. So, in other words, I agree with @mkouba's plan. We would need to document how to skip compression for static files based on the media types (I'm not sure we expose these settings, so we should). Note that compression is handled at the netty level. Basically, we add the netty handler compressing the response. That's why it's global. |
I tend to disagree with that. We had numerous reports/questions about usage of the GZip option and it's especially important if you're paying your bandwidth bills in a cloud environment. |
It's a question of trade off as you use more cpu and so cannot handle the same level of concurrency (which means you may need more instances). I also wanted to add the vertx 4.3 is going to extend how is handled compression. That may give us more flexibility, however that's for June (and I do not believe we can wait) |
Ok, to sum it up:
This would allow a user to:
|
Not that I have seen |
Seems reasonable in theory, but I don't really understand how compression can be used on per resource basis. |
Well, I can imagine a resource with methods that return large JSON payloads and also with a method |
My question was mainly from an implementation point of view. So basically what you are proposing is that we have two different ways of compressing output, one general one based on Vert.x and one custom one? |
Yes, but maybe I'm just overthinking on this issue ;-). The thing is that if we merge the |
It could be useful to look at how web servers do it: https://httpd.apache.org/docs/2.4/mod/mod_deflate.html and https://docs.nginx.com/nginx/admin-guide/web-server/compression/ Apparently, Apache doesn't enable anything by default, but gives two documented options:
Nginx by default will compress only html, but documents the option to compress also xml From that, I gather that we could go the same way and provide an option for compression by mime types: quarkus.http.enable-compression=text/html,text/plain,text/xml,text/css,text/javascript,application/javascript I guess that would accept MIME wildcards too. This applies to static resources as well as endpoints. This seems to be what makes the most sense and is the most flexible, no? quarkus.http.enable-compression=true
# this is the default value, when compression is enabled
#quarkus.http.enable-compression.for=text/html,text/plain,text/xml,text/css,text/javascript,application/javascript Now, sure, we can also support an annotation to make it per endpoint, but IMO that's a "nice to have" and not as important as this sort of config. |
Yeah, this is exactly why I think we should not rush into this... It seems to me like we need to put some more thought into it. |
Well, I like the idea but the problem is that
I don't think the current |
@cescoffier had an idea that we could add |
Closing as this has been addressed in #24558 |
CC @FroMage :)