WIP: Remove com.sun.* dependency #40
Conversation
gorkem
force-pushed
the
new_http
branch
4 times, most recently
from
468c35c to
6cd48ab
Compare
Removes com.sun.* dependency. Uses vert.x as the http server. Infers main class name from manifest.mf Signed-off-by: Gorkem Ercan <[email protected]>
| final JsonObject request = rc.getBodyAsJson(); | ||
| final JsonObject value = request.getJsonObject("value"); | ||
| final JsonParser parser = new JsonParser(); | ||
| final com.google.gson.JsonObject req = parser.parse(value.toString()).getAsJsonObject(); |
There was a problem hiding this comment.
This code does:
- parse body to a
io.vertx.core.json.JsonObject(rc.getBodyAsJson()) - serialize
io.vertx.core.json.JsonObjectto aString(value.toString()) - parse the serialized value to a
com.google.gson.JsonObject(parser.parse())
Looks like unnecessary overhead.
| router.route().failureHandler(ErrorHandler.create(true)); | ||
|
|
||
| router.route("/init").handler(this::initHandler); | ||
| router.route("/run").handler(this::runHandler); |
There was a problem hiding this comment.
Vertx handlers are supposed to execute non-blocking code using an event loop thread pool. By default, vertx even logs warnings if it detects an event loop hasn’t returned for some time. See also https://vertx.io/docs/apidocs/io/vertx/core/VertxOptions.html#DEFAULT_MAX_EVENT_LOOP_EXECUTE_TIME.
Also I'm not sure whether openwhisk runtimes can be invoked concurrently (i.e. to handle multiple events at the same time). If not we should probably tune the VertxOptions to limit the event loop and worker thread pools (e.g. event loop pool has by default 2 * number of cores on the machine).
There was a problem hiding this comment.
Runtimes don’t tolerate intra container concurrency for activations but a PR is coming upstream to make this possible.
| System.setSecurityManager(new WhiskSecurityManager()); | ||
|
|
||
| try { | ||
| final com.google.gson.JsonObject out = (com.google.gson.JsonObject) this.mainMethod.invoke(null, req); |
There was a problem hiding this comment.
i think if you leveraged the vertx features like eventbus the function invocation would be much more reactive.
But, i think new action "kind" would be necessary as the method invocation should make the things a different way,....
what if you invoked function using event bus, e.g.:
vertx.eventBus().<JsonObject>send("activation", <inputJsonObject>, reply - > {
if (reply.failed()) {
rc.response().setStatusCode(500);
} else {
rc.response.end(...);
}
}
Then the function could be just a class extending AbstractVerticle and in start() method simply consume eventBus instead of class with main method.
|
@gorkem @marcinczeczko Any interest in completing this PR? I am personally interested to see both what performance #s we would get as well as trying the concurrency aspect (as this is now configurable in our NodeJS runtime and is used by some downstream providers). |
|
@mrutkows Unfortunately I will not be returning to this one anytime soon. |
|
closing as stale |
Removes com.sun.* dependency. By taking vert.x as the http server. Also starts looking into the manifest.mf to infer the main class name. Manifest.mf still needs work on other repositories to be used. see #14, #12
Update: Just noticed that I broke com.example.MyMain#methodName notation for defining main method. Will update with a fix.