Closed benblack86 closed 6 years ago
Merging #627 into develop will decrease coverage by
0.06%
. The diff coverage is94.73%
.
@@ Coverage Diff @@
## develop #627 +/- ##
===========================================
- Coverage 84.91% 84.84% -0.08%
===========================================
Files 99 99
Lines 4460 4446 -14
Branches 370 356 -14
===========================================
- Hits 3787 3772 -15
- Misses 673 674 +1
Impacted Files | Coverage Δ | |
---|---|---|
...scala/colossus/protocols/websocket/Websocket.scala | 91.3% <ø> (ø) |
:arrow_up: |
...ain/scala/colossus/protocols/http/HttpServer.scala | 73.33% <ø> (-1.67%) |
:arrow_down: |
...s/src/main/scala/colossus/service/ServiceDSL.scala | 83.33% <ø> (-0.35%) |
:arrow_down: |
...rc/main/scala/colossus/service/ServiceServer.scala | 88.88% <0%> (-1.95%) |
:arrow_down: |
...c/main/scala/colossus/service/RequestHandler.scala | 95% <100%> (+5%) |
:arrow_up: |
.../src/main/scala/colossus/core/ServerSettings.scala | 100% <100%> (ø) |
:arrow_up: |
...sus/src/main/scala/colossus/util/ConfigCache.scala | 0% <0%> (-100%) |
:arrow_down: |
...olossus/src/main/scala/colossus/core/Context.scala | 42.85% <0%> (-14.29%) |
:arrow_down: |
...s/src/main/scala/colossus/core/server/Server.scala | 95.13% <0%> (+0.69%) |
:arrow_up: |
...rc/main/scala/colossus/service/ServiceClient.scala | 93.1% <0%> (+1.37%) |
: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 41c6d99...e4e7b2c. Read the comment docs.
They're split because not every Server is a Service. For example, ServiceConfig
is not used when creating a websocket server.
Let me propose an alternative: keep the two case classes separate, but put a ServerSettings
inside of ServiceConfig
. That way the path would be
colossus.service.service-name.server.max-connections = 4000
Perhaps a little long, but at least it would be properly organized and consistent across services.
I think I understand this a little better now. ServiceConfig
is loaded/used by the RequestHandler
and define connection level properties, i.e. how many requests allowed on that connection at any one time.
I think one thing I was getting confused about is all the different terminology for what seems to be the same thing: Service/Request/Connection. Would it make sense to standardize on calling everything a Connection? ConnectionHandler, ConnectionConfig, etc... ?
Having an optional ServiceConfig
inside ServerSettings
would make more sense to me. If a service doesn't need service settings, then it would be none. It would look something like this:
ServerSettings(
port: Int,
maxConnections: Int = 1000,
maxIdleTime: Duration = Duration.Inf,
....,
connection: Some(ConnectionSettings(
requestTimeout: Duration,
connectionBufferSize: Int,
....,
))
)
I agree with @benblack86 I think that having a nested connection settings (using the word connection instead of service since it seems clearer to me). Are we all in agreement on this?
Ok, so I agree with the overall idea of not making the user deal with two separate configs, but I don't agree with this implementation. I think it's coupling things together that shouldn't be coupled. For one thing, it's introducing a circular dependency between the core
and service
packages.
Everything in core
is what's needed to build a raw TCP server or client. It deals with all the connection management and handling raw byte streams. Everything in service
is what adds all of the higher-level logic to build actual services, which includes the notion of requests/responses, retries, timeouts, and load-balancing.
All of the settings in ServiceConfig
have to do with configuring things a the service level, not at the core level. It is true that most of them are per-connection settings, but they are still above what the core layer handles.
I do agree that from a user's perspective that it makes sense to have just one config object. The distinction between core
and service
is mostly hidden from the user (at least a user building a service and not something lower-level).
So maybe we could do something like
ServiceConfig
to ConnectionConfig
- I think that makes senseServiceConfig
that contains 3 things:
a. name
b. a ServerSettings
object
c. a ConnectionConfig
objectThis way, the ServerSettings
can get passed down to the server, the ConnectionConfig
can get handed to each RequestHandler
, and we maintain proper separation of responsibility.
Also, as long as ServerSettings
and ConnectionConfig
don't have identically-named members, I think you can have their Typesafe config objects merged and use the same object in both config parsers, that way the settings could be colossus.service.port
and not colossus.service.server.port
Lastly, I would also be against renaming RequestHandler
to ConnectionHandler
, mostly because I don't think either one perfectly describes what the thing actually is and I'd advise against renaming user-facing things in general unless we're making a bigger change about what it actually does.
Going to close this and play around with @DanSimon suggestion.
Probably another controversial change. Here I am moving
ServiceConfig
intoServerSettings
. This was mainly motived when we ended up with config that looks like this in one of our services:It is not intuitive why some config is part of a "server" and other config is part of a "service". What is the use case for having a distinction between a server and service?
@DanSimon @amotamed @aliyakamercan @jlbelmonte @dxuhuang