Separating digdag-guice-rs-server from digdag-server #316

frsyuki · 2016-09-29T00:34:29Z

Besides moving of code, there're some interface changes for isolation:

digdag-core: BackgroundExecutor interface (=threads that should shutdown before stopping HTTP handlers Graceful shutdown of HTTP server and worker threads #182) is changed to @PreStop annotation. Current code still has BackgroundExecutor interface that has only one method annotated with @PreStop to minimize change of code. Some refactoring may be necessary later.
digdag-server: ServerConfig interface extends UndertowServerConfig interface (nothing changed actually though)
digdag-core: DigdagEmbed.Bootstrap has a method to return org.embulk.guice.Bootstrap rather than Injector so that UndertowBootstrap can add modules before creating injector. I think that DigdagEmbed.Bootstrap should extend org.embulk.guice.Bootstrap but it's a refactoring that may happen later.
digdag-server: Writing ServerRuntimeInfo file is moved to a @PostStart handler (ServerRuntimeInfoWriter).
digdag-cli: cli.Sched and cli.Server create ServerBootstrap instance rather than passing ServerBootstrap.class. This is because UndertowServer takes an instance that implements new shared ServerBootstrap interface.

frsyuki · 2016-09-29T23:28:01Z

nice, worked. dependency conflicts were the last beast. According to ./gradlew classpath result, final dependencies are consistent excepting guice-bootstrap.

$ diff classpath.txt classpath-master.txt  -u
--- classpath.txt       2016-09-29 16:26:48.000000000 -0700
+++ classpath-master.txt        2016-09-29 16:27:00.000000000 -0700
@@ -19,19 +19,17 @@
 commons-io-2.1.jar
 commons-logging-1.2.jar
 cron4j-2.2.5.jar
-digdag-cli-0.8.16-SNAPSHOT.jar
-digdag-client-0.8.16-SNAPSHOT.jar
-digdag-core-0.8.16-SNAPSHOT.jar
-digdag-guice-rs-0.8.16-SNAPSHOT.jar
-digdag-guice-rs-server-0.8.16-SNAPSHOT.jar
-digdag-guice-rs-server-undertow-0.8.16-SNAPSHOT.jar
-digdag-plugin-utils-0.8.16-SNAPSHOT.jar
-digdag-server-0.8.16-SNAPSHOT.jar
-digdag-spi-0.8.16-SNAPSHOT.jar
-digdag-standards-0.8.16-SNAPSHOT.jar
+digdag-cli-0.8.17-SNAPSHOT.jar
+digdag-client-0.8.17-SNAPSHOT.jar
+digdag-core-0.8.17-SNAPSHOT.jar
+digdag-guice-rs-0.8.17-SNAPSHOT.jar
+digdag-plugin-utils-0.8.17-SNAPSHOT.jar
+digdag-server-0.8.17-SNAPSHOT.jar
+digdag-spi-0.8.17-SNAPSHOT.jar
+digdag-standards-0.8.17-SNAPSHOT.jar
 guava-19.0.jar
 guice-4.0.jar
-guice-bootstrap-0.2.1.jar
+guice-bootstrap-0.1.0.jar
 guice-multibindings-4.0.jar
 h2-1.4.192.jar
 hamcrest-core-1.1.jar

@danielnorberg are you comfortable to merge this? It might be better to separate digdag-guice-rs-* (and embulk guice-bootstrap) to another repository and make them stabilized in the future once digdag doesn't need frequent changes in them.

danielnorberg · 2016-09-30T05:52:46Z

digdag-cli/src/main/java/io/digdag/cli/Sched.java

+        public RevisionAutoReloaderStarter(Config systemConfig, ProjectArchiveLoader projectLoader, ConfigFactory cf, RevisionAutoReloader autoReloader)
+        {
+            this.projectDirName = systemConfig.get(SYSTEM_CONFIG_AUTO_LOAD_LOCAL_PROJECT_KEY, String.class);
+            this.overwriteParams = cf.fromJsonString(systemConfig.get(SYSTEM_CONFIG_LOCAL_OVERWRITE_PARAMS, String.class));


s/overwrite/override

oh ok...overwriteParams is used a lot in digdag's code (especially digdag-cli but also some in digdag-core such as ProjectArchiveLoader).
should all of them be replaced?

Probably. But maybe that can be a different PR.

ok. I'll do that in another PR.

danielnorberg · 2016-09-30T05:53:32Z

digdag-core/build.gradle

-    compile 'org.embulk:guice-bootstrap:0.1.0'
-    compile 'com.fasterxml.jackson.module:jackson-module-guice:2.6.7'
-    compile 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.6.7'
+    // this dependency is here only to override dependency confliction of


s/confliction/conflict

danielnorberg · 2016-09-30T05:56:55Z

digdag-server/src/main/java/io/digdag/server/ServerConfig.java

@@ -20,25 +21,33 @@
 @JsonSerialize(as = ImmutableServerConfig.class)
 @JsonDeserialize(as = ImmutableServerConfig.class)
 public interface ServerConfig
+    extends UndertowServerConfig


Should the fact that digdag server uses undertow be part of the public interface?

I think that it's hard to abstract configuration parameters of different HTTP server implementations. Some of them are generic but not all of them. More control is more important than abstraction here in my opinion. Then if we change implementation from Undertow to something else, users will need to change config file anyways. So I think it is OK to expose "Undertow" here.
And more over, although ServerConfig is a public interface, it's not digdag-spi. In my understanding, digdag-server package is an implementation detail of digdag.

danielnorberg · 2016-09-30T06:36:28Z

Remind me, what's the purpose of this PR? Is it just refactoring to improve design/code structure?

On a related note, how come we have such a bespoke http server setup instead of using something more standard like dropwizard?

frsyuki · 2016-09-30T21:10:29Z

This PR is code structure refactoring so that initialization code of Undertow becomes reusable.
Using this setup instead of Dropwizard, Spring boot, or full-stack Java EE including JPA is my preference for simplicity and better understanding of the entire behavior over abstracted magic.
Because I don't have much experience on web API development, having smaller number of external dependencies was also good to make working code as quick as possible. I took ideas from Presto because I'm familiar to its code much more. I still don't feel immediate needs for dignified frameworks honestly.

I think it is ok to separate digdag-server from digdag-cli. Then dependency issues will be smaller. Digdag's server code is based-on JAX-RS. So, migrating to Java EE frameworks seems possible although Spring boot seems more work to adopt. KumuluzEE, Dropwizard, Payara (Micro), Restlet, TomEE, etc.

danielnorberg · 2016-10-03T01:59:10Z

What is the intended user of the undertow initialization code? Some other part of digdag or another project?

If the intent is for the undertow initialization code to become a library reusable by other projects, then perhaps it should be broken out into a separate project without the name digdag? That can be future work in a separate PR though.

I'm comfortable with merging this if you are. 👍

frsyuki · 2016-10-03T18:13:10Z

plazmadb-server is an user: https://github.com/treasure-data/plazmadb/pull/55

Having another repository is good but I'm expecting more changes. It's easier to test changes if code is in this repository. Once digdag doesn't need frequent changes, we can move them.

frsyuki added 7 commits September 27, 2016 16:06

removed unused GuiceRsCommandLineModule

6dd33a8

Separate digdag-guice-rs-server from digdag-server

2bfea9f

digdag-guice-rs-server-undertow needs only jackson-core

3191c7e

update version of guice-bootstrap to 0.2.1

b9e44c4

removed unnecessary direct dependencies

55da513

Exclude dependency from jackson-module-guice to guice 3.0

a94d4e4

FasterXML/jackson-modules-base#22

override dependency conflict of jboss-logging

65197d6

frsyuki changed the title ~~[WIP] Separating digdag-guice-rs-server from digdag-server~~ Separating digdag-guice-rs-server from digdag-server Sep 29, 2016

danielnorberg reviewed Sep 30, 2016

View reviewed changes

typo

d1c32b0

frsyuki merged commit 4f65bb8 into master Oct 3, 2016

frsyuki deleted the server-module branch October 3, 2016 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separating digdag-guice-rs-server from digdag-server #316

Separating digdag-guice-rs-server from digdag-server #316

frsyuki commented Sep 29, 2016 •

edited

frsyuki commented Sep 29, 2016

danielnorberg Sep 30, 2016

frsyuki Sep 30, 2016

danielnorberg Sep 30, 2016

frsyuki Sep 30, 2016

danielnorberg Sep 30, 2016

frsyuki Sep 30, 2016 •

edited

danielnorberg Sep 30, 2016

frsyuki Sep 30, 2016

danielnorberg Sep 30, 2016

danielnorberg commented Sep 30, 2016

frsyuki commented Sep 30, 2016 •

edited

danielnorberg commented Oct 3, 2016 •

edited

frsyuki commented Oct 3, 2016 •

edited

Separating digdag-guice-rs-server from digdag-server #316

Separating digdag-guice-rs-server from digdag-server #316

Conversation

frsyuki commented Sep 29, 2016 • edited

frsyuki commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frsyuki Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnorberg commented Sep 30, 2016

frsyuki commented Sep 30, 2016 • edited

danielnorberg commented Oct 3, 2016 • edited

frsyuki commented Oct 3, 2016 • edited

frsyuki commented Sep 29, 2016 •

edited

frsyuki Sep 30, 2016 •

edited

frsyuki commented Sep 30, 2016 •

edited

danielnorberg commented Oct 3, 2016 •

edited

frsyuki commented Oct 3, 2016 •

edited