Closed dxuhuang closed 7 years ago
Wondering if the maxInitBufferSize
parameter to bytes()
should also come from ServiceConfig.requestBufferSize
. If not, I will probably just pass in the maxRequestSize
and not the whole ServiceConfig
Merging #549 into develop will increase coverage by
0.02%
. The diff coverage is100%
.
@@ Coverage Diff @@
## develop #549 +/- ##
===========================================
+ Coverage 84.83% 84.85% +0.02%
===========================================
Files 98 98
Lines 4312 4313 +1
Branches 342 349 +7
===========================================
+ Hits 3658 3660 +2
+ Misses 654 653 -1
Impacted Files | Coverage Δ | |
---|---|---|
...rc/main/scala/colossus/controller/Controller.scala | 83.33% <ø> (ø) |
:arrow_up: |
...ain/scala/colossus/protocols/http/HttpServer.scala | 68.75% <100%> (ø) |
:arrow_up: |
...cala/colossus/protocols/http/HttpServerCodec.scala | 80% <100%> (ø) |
:arrow_up: |
...la/colossus/protocols/http/HttpRequestParser.scala | 95.83% <100%> (+0.18%) |
:arrow_up: |
...cala/colossus/protocols/http/HttpClientCodec.scala | 40% <100%> (ø) |
:arrow_up: |
.../scala/colossus/metrics/collectors/Histogram.scala | 92.64% <0%> (+0.73%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 10903b7...0d399b9. Read the comment docs.
ServiceConfig.maxRequestSize
is passed into ServiceServer.controllerConfig.inputMaxSize
. Should we change reference.conf
accordingly?
My initial thoughts are yes we should add to reference.conf
and no to maxInitBufferSize
since I think we should leave that until people ask for it (hopefully never?!). I think passing ServiceConfig
is fine since it is passed by reference so there should be no performance difference between passing an int
and a ServiceConfig
object (unless cpu memory is affected?).
So ServiceConfig.requestBufferSize
is not the size of a byte buffer, but rather the maximum number of pending requests for a single connection before the connection starts automatically failing requests. The Http parsers won't need that value.
But maxInputBufferSize
in bytes()
and inputMaxSize
in ControllerConfig
should both be set to ServiceConfig.maxRequestSize
.
So you should not have to pass in the entire ServiceConfig
to the parsers. I don't think that should be done anyway since the parser doesn't need to know about any of the other settings and we should always aim to keep things as focused as possible. If in the future a parser needs multiple values from the config, we would group those into another case class inside of ServiceConfig
.
@DanSimon In Parsing.scala
:
def bytes(num: Int, maxSize: DataSize, maxInitBufferSize: DataSize): Parser[Array[Byte]] = { ... }
Which parameter did you mean by maxInputBufferSize
? If it's maxInitBufferSize
, I recall you said a while ago that this should be small, which is why it's currently hardcoded to 1 KB (https://github.com/tumblr/colossus/pull/505).
Ah yeah I misread, I meant maxSize
when talking about coming from config. You're correct that maxInitBufferSize
should be small and can stay hardcoded at 1KB. It could potentially be an issue if a service is constantly parsing large (multi-MB) requests, but would need some experimentation to be really sure before we make it configurable.
bump
bump, added docs
@DanSimon @benblack86 @jlbelmonte @aliyakamercan @amotamed
https://github.com/tumblr/colossus/issues/542