Skip to content
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

use service loader with gziphandler #9199

Closed
wants to merge 3 commits into from

Conversation

jimexist
Copy link

@joakime
Copy link
Contributor

joakime commented Jan 23, 2023

There's no test cases show how this would be used.

Can you create a ExampleCompression whatever in the src/test/java and src/test/resources/META-INF/services/ to show it working? The compression doesn't have to do actual compression, heck, make it just ROT13 for purposes of the test. (that way we can see it performing the transformation and back again)

@@ -876,7 +878,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
shopContext.setHandler(new ShopHandler());

// You want to gzip the shop web application only.
GzipHandler shopGzipHandler = new GzipHandler();
DynamicCompressionHandler shopGzipHandler = ServiceLoader.load(DynamicCompressionHandler.class).iterator().next();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see the benefit of using the ServiceLoader this way. A user can just extend GzipHandler and then call ServletContextHandler.setGzipHandler.

Can you explain the problem you're trying to solve a bit more?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specific approach inspired by original issue #8769 but personally i think it's okay to have it either way as long as interface is consistent

@gregw
Copy link
Contributor

gregw commented Feb 15, 2023

Sorry we have not acted on this PR for while. Busy month.
Will tune in next month.

@jimexist jimexist closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce CompressionHandler to support compression from gzip, brotli, and zstandard
4 participants