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

[PUBDEV-4271] H2O Modularization #915

Merged
merged 2 commits into from May 26, 2017
Merged

[PUBDEV-4271] H2O Modularization #915

merged 2 commits into from May 26, 2017

Conversation

mmalohlava
Copy link
Member

This is WIP !

Copy link
Contributor

@vpatryshev vpatryshev left a comment

Choose a reason for hiding this comment

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

Kind of weird that there are 0 tests here.

@mmalohlava
Copy link
Member Author

@vpatryshev this is WIP, i just would like too see status of jenkins run.

@jakubhava
Copy link
Contributor

Just so we don't forget about it. The capabilities rest endpoint is implemented in https://github.com/h2oai/h2o-3/tree/jh/exp/modularization-new and the commits can be cherry-picked from there

@mmalohlava
Copy link
Member Author

@jakubhava thanks! I put it into the branch.

@mmalohlava mmalohlava force-pushed the mm/exp/modularization branch 2 times, most recently from 2ba113a to 7e810a5 Compare May 24, 2017 00:24
@mmalohlava mmalohlava changed the title WIP: on the way of modularization [PUBDEV-4271] H2O Modularization May 24, 2017
@mmalohlava
Copy link
Member Author

@h2o-ops please test!

@@ -834,40 +842,40 @@ public static void registerRestApis(String relativeResourcePath) {
}

// Log extension registrations here so the message is grouped in the right spot.
for (AbstractH2OExtension e : H2O.getExtensions()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why is this are the initialization messages printed in this method (it doesn't seem to have anything to do with the extensions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, i can remove it but i did not want to break compatibility of interface.

@@ -1613,12 +1621,13 @@ private static void startNetworkServices() {
}

// Callbacks to add new Requests & menu items
// FIXME: remove me please
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

@michalkurka michalkurka left a comment

Choose a reason for hiding this comment

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

LGTM; minor comments

mmalohlava and others added 2 commits May 25, 2017 17:25
This change contains two main code changes:
  - using SPI to register core extensions and REST API extension
  - removing use of reflection library for module lookup

  - We would like to provide more flexible system to extend H2O and plug new tools into the H2O platform (e.g, XGBoost, TensorFlow, Sparkling Water).
  - The current code base is using [reflections library](https://github.com/ronmamo/reflections) to handle lookup of optional components, however it brings several issues including:
    - limit on used package name by extension (only `water` and `hex` are allowed)
    - force traversal of full classpath which causes problems in systems with dynamic classloaders (e.g., Spark executors).

  - We will remove usage of reflections library to find instances of `water.AbstractH2OExtension`, `water.api.AbstractRegister` and `water.api.Schema`
  - The extensions (meaning classes listed in the previous point) will be registered using [Java Service Provider Interface](https://docs.oracle.com/javase/tutorial/sound/SPI-intro.html). In short, the concept relies on service files that are located in `META-INF/services` directory. Each service file is called by the name of a class it extends (e.g., `water.AbstractH2OExtension`) and contains a list of classes that extend the service class. For example, for core H2O REST API we have a single file `h2o-core/src/main/resources/META-INF/services/water.api.RestApiExtension` which contains 3 REST API extensions implementing interface `water.api.RestApiExtension`:
  ```
  water.api.RegisterResourceRoots
  water.api.RegisterV3Api
  water.api.RegisterV4Api
  ```

  - We provide capabilities REST end-point to provide list of registered core extensions, REST API extensions, parsers (WIP)

  - In the scope of H2O source code, we provide optional `@AutoService` annotation to register extensions (see [documentation](https://github.com/google/auto/tree/master/service)).

  - We do not modularize R/Python/Flow clients. The client is responsible to self-configure based on information provided by the backend and fails gracefully if the user invokes an operation that is not provided by backend

> Note: the same concept is already used in H2O to register parsers and Rapids extensions.

  - Code that register new REST API calls by extending `water.api.AbstractRegister` class will need to be updated by adding a service file as described above
  - Each class extending `water.api.Schema` needs to be registered as well in `water.api.Schema` service file.

----

History of changes
 - apt schema generation SPI files.
 - Proper schema registration
 - Service files
 - Fix service files
 - Schema registration as part of RestAPI registration.
 - AutoML modularized
 - Boot message provides a list of registered extensions for core and REST API.
 - Pick auto-service configuration from Kuba's branch.
 - Proper name for Rest API extensions.
 - Append missing schema into list of schemas.
 - More schemas.
 - Do not confuse test infrastructure by non-test files.
 - Integrating review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants