FroMage / redpipe

Redpipe Web Framework
Apache License 2.0
70 stars 10 forks source link

Server start method - why don't use conf/config.json? #11

Closed fiorenzino closed 6 years ago

fiorenzino commented 6 years ago

Reading the net.redpipe.engine.core.Server code, the vertx instance is always started using no options:

VertxOptions options = new VertxOptions(); options.setWarningExceptionTime(9223372036854775807L); this.vertx = Vertx.vertx(options);

We could use the "conf/config.json" properties to init vertx: VertxOptions vertxOptions = new VertxOptions(jsonObject);

and like said VertxOptionsConverter.fromJson, we can configure all the vertx options: addressResolverOptions, blockedThreadCheckInterval, clusterHost, clusterPingInterval, clusterPingReplyInterval, clusterPort, clusterPublicHost, clusterPublicPort, clustered, eventBusOptions, eventLoopPoolSize, fileResolverCachingEnabled, haEnabled, haGroup, internalBlockingPoolSize, maxEventLoopExecuteTime, maxWorkerExecuteTime, metricsOptions, preferNativeTransport, quorumSize, warningExceptionTime, workerPoolSize

FroMage commented 6 years ago

Very good idea!

fiorenzino commented 6 years ago

I created my implementation of your Server class (using the code inside io.vertx.core.Starter class): https://github.com/fiorenzino/redpipe/blob/REDPIPE12/redpipe-engine/src/main/java/net/redpipe/engine/core/Server.java

My Server version, use the config.json to initialize vertx ( i changed your code to scan the config file in java way, without use the vertx filesystem, beacause vertx will be started using the same file).

But we have two problems:

I think we should use a more robust vertx start mechanism like io.vertx.core.Starter.

WDYT?

FroMage commented 6 years ago

Sorry I can't really check the diff because you changed indentation, so I'm not sure what changes you made.

I think the conf/config.json should be resolved in the CWD of the current application, not in the root project. That's what makes the most sense.

Otherwise Starter is deprecated, and seems to be a main entry point, which is not going to work for us, no?

fiorenzino commented 6 years ago

The 2 principal changes in my code: 1 - load file config using java Scanner instead of vertx.filesystem (because the vertx will start using the config.json options) private Single loadFileConfig(JsonObject config) { if (config != null) { return save(config); } String confArg = "conf/config.json"; File file = new File(confArg); System.out.println(file.getAbsolutePath()); try (Scanner scanner = new Scanner(new File(confArg)).useDelimiter("\A")) { String sconf = scanner.next(); try { return save(new JsonObject(sconf)); } catch (DecodeException e) { log.error("Configuration file " + sconf + " does not contain a valid JSON object"); // empty config return save(new JsonObject()); } } catch (FileNotFoundException e) { return save(new JsonObject()); } }

2 - init vertx using the file config.json translated in JsonObject:

private Single initVertx(JsonObject config) { VertxOptions options; if (config != null) { options = new VertxOptions(config); } else { options = new VertxOptions(); } options.setWarningExceptionTime(Long.MAX_VALUE); vertx = Vertx.vertx(options); AppGlobals.get().setVertx(vertx); return Single.just(config); } The limit of this method is: vertx = Vertx.vertx(options); If the options clustered=true, we need use: AtomicReference<AsyncResult> result = new AtomicReference<>(); Vertx.clusteredVertx(options, ar -> { result.set(ar); }); vertx = Vertx.vertx(options);

The Starter (deprecated) or the Launcher, are the equivalent of our Server: they are the deployer of Verticles. In our case, we need only the code where they run the Vertx instance, using the configuration passed to the Server. The method complete, interesting for us is: io.vertx.core.Starter.startVertx(boolean clustered, boolean ha, Args args)

Can you share your code formatter profile? In some project i use https://github.com/forge/core/blob/master/eclipse-code-formatter-profile.xml

FroMage commented 6 years ago

I'm using the default Eclipse formatting: one tab per level.

FroMage commented 6 years ago

Closing as it is now supported, I also added some docs. Thanks a lot!