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

Spike: Simplify styx servers and inject executors #578

Closed
wants to merge 11 commits into from

Conversation

mikkokar
Copy link
Contributor

@mikkokar mikkokar commented Dec 23, 2019

Netty Servers Spike

Goal is to prepare core styx classes to accommodate http2 server implementation.

Server Configuration

Utilise existing Styx Config Object model and its underpinning framework for configuring Styx servers. This involves adding a new servers configuration map like so:

servers:
  http:
    type: HttpServer
    config:
      port: 8081
      handler: hello2

  h2:
    type: Http2Server
    config:
      port: 8443
      handler: hello2

routingObjects:
  hello2:
    type: StaticResponseHandler
    config:
      status: 200
      content: "hello-2"

Styx http2 will eventually be implemented as a Http2Server server object.

Benefits:

  • Styx is no longer limited to a pair of http or https endpoints. Users specify as many servers as they want.

  • Simplify & reduce Styx server implementation. We will handle routing objects, control plane providers, and Styx servers with same underlying framework. Thus we can cut down any bespoke code that is specific to the styx servers alone.

  • (PR Refactor ProviderObjectRecord #582) As a part of this work, the ProviderObjectRecord has merged into a new StyxObject<T> as follows :

internal data class StyxObjectRecord<T : StyxService>(
        val type: String,
        val tags: Set<String>,
        val config: JsonNode,
        val styxService: T)

internal typealias ProviderObjectRecord = StyxObjectRecord<StyxService>

Netty Executors

(PR #583)

I have introduced new NettyExecutor class. It encapsulates a Netty event loop, and it corresponding client and server channel types. NettyExecutor.create attempts to use an Epoll when available, and it reverts to NIO when not available. This replaces the functionality that used to be scattered across ClientEventLoopFactory, EpollClientEventLoopFactory, NioEventLoopGroupFactory, and PlatformAwareEventLoopGroupFactory classes.

We can now read the thread pool configuration etc in the main method from the styx configuration, instantiate executors accordingly, and inject them in to the class hierarchy.

This reduces a need to pass styx configuration across several components.

Other improvements

ProxyServerbuilder

  • Removed. The proxy server can be instantiated almost as easily from NettyServer directly.

PR: #581

NettyServer

  • Remove ServerListener and ServerSocketBinder inner classes. This was made possible by earlier work to restrict only one server socket binder per NettyServer.

PR: #580

@mikkokar mikkokar changed the title Simplify styx servers and inject executors Spike: Simplify styx servers and inject executors Jan 10, 2020

import static com.hotels.styx.StyxConfig.NO_JVM_ROUTE_SET;
import com.hotels.styx.api.LiveHttpRequest;

/**
* Formats response info into a string.
*/

// TODO: Should be package private, but it is shared by com.hotels.styx.servers.StyxHttpServer.kt
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need a TODO here. It's a trivial problem and the TODO will be there forever and will never get fixed. Better would be to fix it by moving StyxHttpServer to com.hotels.styx package or just remove comment.

field("port", integer()),
field("handler", string())

// optional("tlsSettings", `object`(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to submit commented out code?


class StyxHttpServerTest : StringSpec() {

init {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to commit an empty test?

* @return event loop group
*/
EventLoopGroup newBossEventLoopGroup();
public interface IStyxServer extends StyxService {
Copy link
Contributor

@OwenLindsell OwenLindsell Jan 13, 2020

Choose a reason for hiding this comment

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

Why do we need to create this interface? only one class implements it

}
public Void call() {
channelGroup.close().awaitUninterruptibly();
// TODO: Mikko: check why the return value is never used:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should your name be in the code? :)

@mikkokar mikkokar closed this Jan 13, 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