Skip to content

Update to jetty 9.4; Enable request decompression#5624

Merged
jon-wei merged 2 commits intoapache:masterfrom
zhztheplayer:request-decompress
Jun 8, 2018
Merged

Update to jetty 9.4; Enable request decompression#5624
jon-wei merged 2 commits intoapache:masterfrom
zhztheplayer:request-decompress

Conversation

@zhztheplayer
Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer commented Apr 11, 2018

Automatic request decompression is implemented since Jetty-964，to use this feature, a dependency upgrading of jetty from 9.3 to 9.4 is needed.

nishantmonu51
nishantmonu51 reviewed Apr 11, 2018
View reviewed changes
Comment thread server/src/main/java/io/druid/server/initialization/jetty/JettyServerInitUtils.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Apr 11, 2018

Choose a reason for hiding this comment

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

does the buffer size needs to be configurable ?

Copy link
Copy Markdown
Member Author

@zhztheplayer zhztheplayer Apr 12, 2018

Choose a reason for hiding this comment

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

+1

@zhztheplayer zhztheplayer force-pushed the request-decompress branch 3 times, most recently from 64cebd9 to eb0e9a2 Compare April 12, 2018 09:47
@zhztheplayer
Copy link
Copy Markdown
Member Author

zhztheplayer commented Apr 12, 2018

@nishantmonu51 the buffer size is now configurable, and add an compression level option to solve #5534

Also added docs and unit tests.

@zhztheplayer zhztheplayer force-pushed the request-decompress branch 2 times, most recently from 97ed14a to e213caa Compare April 12, 2018 11:22
@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Apr 17, 2018

Please check the integration test failure.

[ERROR] Tests run: 11, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 1,942.477 s <<< FAILURE! - in TestSuite
[ERROR] testRealtimeIndexTask(io.druid.tests.indexer.ITAppenderatorDriverRealtimeIndexTaskTest)  Time elapsed: 305.888 s  <<< FAILURE!
io.druid.java.util.common.ISE: 
Error while querying[http://172.17.0.1:8888/druid/v2/] status[502 Bad Gateway] content[<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 502 Bad Gateway</title>
</head>
<body><h2>HTTP ERROR 502</h2>
<p>Problem accessing /druid/v2. Reason:
<pre>    Bad Gateway</pre></p><hr><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.4.9.v20180320</a><hr/>
</body>
</html>
]
	at io.druid.tests.indexer.ITAppenderatorDriverRealtimeIndexTaskTest.testRealtimeIndexTask(ITAppenderatorDriverRealtimeIndexTaskTest.java:55)
[ERROR] testRealtimeIndexTask(io.druid.tests.indexer.ITRealtimeIndexTaskTest)  Time elapsed: 305.205 s  <<< FAILURE!
io.druid.java.util.common.ISE: 
Error while querying[http://172.17.0.1:8888/druid/v2/] status[502 Bad Gateway] content[<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 502 Bad Gateway</title>
</head>
<body><h2>HTTP ERROR 502</h2>
<p>Problem accessing /druid/v2. Reason:
<pre>    Bad Gateway</pre></p><hr><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.4.9.v20180320</a><hr/>
</body>
</html>
]
	at io.druid.tests.indexer.ITRealtimeIndexTaskTest.testRealtimeIndexTask(ITRealtimeIndexTaskTest.java:56)

@zhztheplayer zhztheplayer force-pushed the request-decompress branch 5 times, most recently from 2ad349c to 5daa683 Compare May 9, 2018 06:22
@zhztheplayer
Copy link
Copy Markdown
Member Author

zhztheplayer commented May 29, 2018 •
edited
Loading

@jihoonson @nishantmonu51
Looks like a mismatch of incoming http request between header field Content-Length and the actual content length could be produced from router node after the request de-jsonization and re-jsonization.

I leaved a quick fix in the newest commit of the PR.

jon-wei
jon-wei reviewed Jun 7, 2018
View reviewed changes
Comment thread server/src/test/java/io/druid/server/initialization/JettyTest.java Outdated
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei Jun 7, 2018

Choose a reason for hiding this comment

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

Can you add a test where the request has Accept-Encoding:gzip and the server sends a gzip compressed response back?

Copy link
Copy Markdown
Member Author

@zhztheplayer zhztheplayer Jun 8, 2018

Choose a reason for hiding this comment

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

@jon-wei
+1 This is supposed to be done by existing method testGzipCompression(), I added some content checking asserts to that method.

Also changed the relevant test method name to avoid confusing.

Comment thread server/src/main/java/io/druid/server/initialization/ServerConfig.java Outdated
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei Jun 7, 2018

Choose a reason for hiding this comment

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

Can you make 4096 a static constant in ServerConfig, maybe DEFAULT_GZIP_INFLATE_BUFFER_SIZE

Copy link
Copy Markdown
Member Author

@zhztheplayer zhztheplayer Jun 8, 2018

Choose a reason for hiding this comment

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

+1 constant added

@jon-wei jon-wei merged commit cfa94b7 into apache:master Jun 8, 2018
jon-wei pushed a commit to implydata/druid-public that referenced this pull request Sep 20, 2018
@zhztheplayer @jon-wei
Update to jetty 9.4; Enable request decompression (apache#5624)
5923e50 
* Update to jetty 9.4; Enable request decompression; Add http compression config options

* Fix BadMessageException from jetty server at HttpGenerator.generateHeaders(...)
@dclim dclim added this to the 0.13.0 milestone Oct 8, 2018
leventov
leventov reviewed Oct 22, 2018
View reviewed changes
Comment thread server/src/test/java/io/druid/server/AsyncManagementForwardingServletTest.java

final HandlerList handlerList = new HandlerList();
handlerList.setHandlers(new Handler[]{JettyServerInitUtils.wrapWithDefaultGzipHandler(root)});
handlerList.setHandlers(new Handler[]{JettyServerInitUtils.wrapWithDefaultGzipHandler(root, 4096, -1)});
Copy link
Copy Markdown
Member

@leventov leventov Oct 22, 2018

Choose a reason for hiding this comment

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

Should have used ServerConfig.DEFAULT_GZIP_INFLATE_BUFFER_SIZE and Deflater.DEFAULT_COMPRESSION instead of magic constants

This was referenced Feb 20, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leventov leventov leventov left review comments

@nishantmonu51 nishantmonu51 nishantmonu51 left review comments

@jon-wei jon-wei jon-wei approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.13.0

Development

Successfully merging this pull request may close these issues.

6 participants

@zhztheplayer @jihoonson @leventov @nishantmonu51 @jon-wei @dclim