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

Add HttpServer object #591

Merged
merged 7 commits into from
Jan 16, 2020
Merged

Conversation

mikkokar
Copy link
Contributor

@mikkokar mikkokar commented Jan 15, 2020

Description

Adds a HttpServer configuration object.

Also addresses #408.

  • Now you can start as many servers as necessary, each with different configuration.
  • They don't use that global httpHandler attribute. Each object have their own handler attribute.
  • The old proxy is still mandatory, and would have to be deprecated in a separate increment.
  • The old proxy is no longer mandatory. It can still be used for setting the global attributes like client thread pool size. In this case it is not necessary to set any connectors.

An example usage:

    servers:
      myHttp:
        type: HttpServer
        config:
          port: 0
          handler: nonSecure
                        
      myHttps:
        type: HttpServer
        config:
          port: 0
          handler: secure
          tlsSettings:
            certificateFile: $crtFile
            certificateKeyFile: $keyFile
            sslProvider: JDK

    routingObjects:
      secure:
        type: StaticResponseHandler
        config:
          status: 200
          content: "secure"
                      
      nonSecure:
        type: StaticResponseHandler
        config:
          status: 200
          content: "non-secure"

Configuration

val SCHEMA = `object`(
        field("port", integer()),
        field("handler", string()),
        optional("tlsSettings", `object`(
                optional("sslProvider", string()),
                optional("certificateFile", string()),
                optional("certificateKeyFile", string()),
                optional("sessionTimeoutMillis", integer()),
                optional("sessionCacheSize", integer()),
                optional("cipherSuites", list(string())),
                optional("protocols", list(string()))
        )),

        optional("maxInitialLength", integer()),
        optional("maxHeaderSize", integer()),
        optional("maxChunkSize", integer()),

        optional("requestTimeoutMillis", integer()),
        optional("keepAliveTimeoutMillis", integer()),
        optional("maxConnectionsCount", integer()),

        optional("bossThreadsCount", integer()),
        optional("workerThreadsCount", integer())
)

Future improvements

  • Share the Netty executor with the server objects.
  • Implement admin server as a server object.
  • Deprecate proxy and httpHandler configuration attributes.
  • Default parameter values should be consistent with the rest of the system?

@mikkokar
Copy link
Contributor Author

Update: Pushed a new commit to deprecate proxy attribute. It still can be specified, but it is no longer mandatory.

@@ -115,6 +116,7 @@
field("factories", map(object(opaque())))
)),
optional("providers", map(routingObject())),
optional("servers", map(routingObject())),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy with servers or do we want to go to something more common like listeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should think about the naming before freezing this. However listeners sound bit like our routing objects. In fact I was thinking to rename routingObjects to listeners. Then you would have servers that consume traffic from the network, and listeners to process the traffic.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've already discussed this and it seems we can keep using our current names.

@@ -70,7 +70,7 @@

STYX_SERVER_CONFIGURATION_SCHEMA_BUILDER = newDocument()
.rootSchema(object(
field("proxy", object(
optional("proxy", object(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to move at least the compressResponses attribute somewhere else if it is optional now.
Don't we need any other of the attributes? factories, bossThreadsCount ... It seems bossThreadsCount was always applied to each listener/ServerBooStrap individually (we aren't sharing the NioEventLoopGroup between different BootStraps).

Copy link
Contributor Author

@mikkokar mikkokar Jan 16, 2020

Choose a reason for hiding this comment

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

  1. compressResponses attribute, and the other missing attributes would still work with the old proxy attribute. I will add them as a separate increment to the server object. I can do it straight after the PR is merged.

  2. Regarding The executors & thread counts. This is a know limitation and I think mentioned in the PR description. We need to think about a specific mechanism of configuring and injecting event loops into various kinks of styx objects: servers, routing objects, and providers. I'm thinking to introduce a separate executors block where you declare named executors, which can be then called into action from the other styx objects. For example:

executors:
  ioClient:
    type: NettyEventloop
    config:
       threads: 4
       channelType: client

  ioServer:
    type: NettyEventloop
    config:
      thread: 3
      channelType: server

servers:
  myServer:
    type: HttpServer
    config:
      port: 8080
      handler: hostProxy
      eventLoop: ioServer

routintObjects:
  hostProxy:
    type: HostProxy
    config:
       host: abc:8080
       eventLoop: ioClient

(This is a separate enhancement BTW)

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised an issue for missing content compression: #593

@mikkokar mikkokar merged commit 933db8b into ExpediaGroup:master Jan 16, 2020
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.

2 participants