Skip to content

Bump web server max header size to accommodate long urls#11951

Open
cbalci wants to merge 2 commits intoapache:masterfrom
cbalci:web-server-max-header-size
Open

Bump web server max header size to accommodate long urls#11951
cbalci wants to merge 2 commits intoapache:masterfrom
cbalci:web-server-max-header-size

Conversation

@cbalci
Copy link
Contributor

@cbalci cbalci commented Nov 3, 2023

There are some endpoints (e.g. broker API /debug/routingTable/sql) which accept SQL as query string param. Default limit of 8K bytes can be too little for long queries so I'm bumping it to a more reasonable value, 64K.

Ideally this should be fixed by moving the parameter to request body (POST), but this change is also needed to help older clients.

bugfix

@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2023

Codecov Report

Merging #11951 (64d1946) into master (2dca141) will increase coverage by 0.06%.
Report is 64 commits behind head on master.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #11951      +/-   ##
============================================
+ Coverage     61.37%   61.44%   +0.06%     
+ Complexity     1147      207     -940     
============================================
  Files          2373     2385      +12     
  Lines        128265   129199     +934     
  Branches      19799    20000     +201     
============================================
+ Hits          78722    79382     +660     
- Misses        43850    44062     +212     
- Partials       5693     5755      +62     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 61.37% <0.00%> (+0.05%) ⬆️
java-21 61.31% <0.00%> (+0.05%) ⬆️
skip-bytebuffers-false 61.40% <0.00%> (+0.04%) ⬆️
skip-bytebuffers-true 61.28% <0.00%> (+0.05%) ⬆️
temurin 61.44% <0.00%> (+0.06%) ⬆️
unittests 61.43% <0.00%> (+0.06%) ⬆️
unittests1 46.67% <0.00%> (+0.03%) ⬆️
unittests2 27.61% <0.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...org/apache/pinot/core/util/ListenerConfigUtil.java 0.00% <0.00%> (ø)

... and 139 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Please take a look at the test failure.

Is the query sent by HTTP header? Why are we setting such a large HTTP header size?

@cbalci
Copy link
Contributor Author

cbalci commented Nov 9, 2023

Thanks @Jackie-Jiang !

Please take a look at the test failure.

Fixed the test case to reflect the updated URL size limit. Though, I'm not sure if this test serves a meaningful purpose. I can get rid of the case if you agree.

Is the query sent by HTTP header? Why are we setting such a large HTTP header size?

It is not the actual HTTP header which causes the problem but "URL + query string" size. URL + query string is counted towards the maxHttpHeaderSize and URL's such as /debug/routingTable/sql?query=<SQL> can fail.

@Jackie-Jiang
Copy link
Contributor

What is the side effect of this change? I'd assume it requires broker to allocate larger buffer for each request handling thread? Will it potentially cause memory issue?

@Jackie-Jiang
Copy link
Contributor

For a query longer than 8K bytes, IMO we should not allow it to be posted along with the URL.
This change will be applied to all rest APIs, which could cause much higher overhead

@xiangfu0 xiangfu0 removed the bugfix label Mar 20, 2026
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.

4 participants