BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint #278

yzang · 2017-07-24T19:07:34Z

Add Bookkeeper Http Server as a new module [bookkeeper-http]

Add children module [http-server], which provide generic api for setting up a Http Server for bookkeeper, allowing to plugin different Http frameworks and inject the implementation of different BK related services to Http Handler to handle different endpoints
Add children module [vertx-http-server], which is a Vertx.io based http server implementation
Add children module [twitter-http-server], which is a TwitterServer based http server implementation
Add a package or org.apache.bookkeeper.http in [bookkeeper-server], which include all the implementation for the services that handle different http endpoint request.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
<Issue # or BOOKKEEPER-#>: Description of pull request
e.g. Issue 123: Description ...
e.g. BOOKKEEPER-1234: Description ...

Make sure tests pass via mvn clean apache-rat:check install findbugs:check.

Replace <Issue # or BOOKKEEPER-#> in the title with the actual Issue/JIRA number.

Provide a general interface for HttpServer, which can be easily implemented by different HTTP frameworks. This change include two implementations of HttpServer, one is TwitterServer and another one is Vertx. Provide a general AbstractHandlerFactory, which is able to create handlers to handler different http apis, and is able to be implemented for different HTTP frameworks. Provide a service level classes, which include all the logics that http handlers will use. Add test cases to test HttpServer

[http-server] is a child module which provide generic http server, http handlers and http service APIs. [vertx-http-server] and [twitter-http-server] are two implementations of http server.

sijie

LGTM +1

sijie

actually I think there are a few checkstyle errors in twitter-http-server module. @yzang do you mind taking a look into it?

yzang · 2017-07-25T00:53:39Z

Sure, I'll address the styles issues

eolivelli

Some comments.
Overall the approach is good.
I would like to try to implement a Servlet based http server.
I wonder if we really need to implement so many variants of the http server or simply implement one

eolivelli · 2017-07-25T14:53:12Z

bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServerLoader.java

+        String className = conf.getString(HTTP_SERVER_CLASS);
+        if (className != null) {
+            try {
+                Class cls = Class.forName(className);


we should use ReflectionUtils but I think it is not possible as it is in bookkeeper-server
@sijie should we create a new minor project "bookeeper-utils" ?

@eolivelli we should refactor the module layout, but I would defer that to next release.

eolivelli · 2017-07-25T14:54:24Z

...keeper-http/http-server/src/main/java/org/apache/bookkeeper/http/service/ServiceRequest.java

+public class ServiceRequest {
+    private String body;
+    private HttpServer.Method method = HttpServer.Method.GET;
+    private Map params = new HashMap();


it is better to declare explicit types of the param, maybe <String,String> o <String, String[]>

eolivelli · 2017-07-25T14:56:31Z

...eper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/package-info.java

+ * the License.
+ */
+/**
+ * @TODO: Write JavaDoc comment {@link https://github.com/apache/bookkepeer/issues/247}


link is broken, anyway we just fixed issue #247 so please add a minimal javadoc

Sure, I'll add some javadoc

eolivelli · 2017-07-25T14:56:52Z

bookkeeper-server/pom.xml

+    <dependency>
+      <groupId>org.apache.bookkeeper.http</groupId>
+      <artifactId>http-server</artifactId>
+      <version>4.5.0-SNAPSHOT</version>


use ${project.version}

Ah, good catch, I'll fix it.

eolivelli · 2017-07-25T14:59:49Z

bookkeeper-server/pom.xml

+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.json</groupId>


please consider using http://json-b.net/ which is the current (brand new) standard for Java-JSON mapping.
org.json is really bad.
otherwise we could use Jackson Mapper, which is actually the best

sure, I'll try jsonb or jackson, thanks.

eolivelli · 2017-07-25T15:00:17Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/http/ConfigurationService.java

+    public ServiceResponse handle(ServiceRequest request) {
+        ServiceResponse response = new ServiceResponse();
+        Map configMap = toMap(conf);
+        JSONObject jsonResponse = new JSONObject();


see previous comment about not using org.json legacy library

sijie · 2017-07-25T16:16:15Z

"I wonder if we really need to implement so many variants of the http server or simply implement one"

different organizations might have different preferences, just like stats providers. so I do not see a problem for that. a simple implementation can be the default one.

dcastor · 2017-07-27T06:11:08Z

bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServer.java

+ * It also provide the interface to inject service provider,
+ * which is the implementation of services for http endpoints.
+ */
+public interface HttpServer {


I think it'd be good to return a list of available endpoints here. That way you can check to see what all is available and its corresponding endpoint. Something like "getEndpoints()".

Are you suggesting adding a service to return all endpoints so that people can curl to see all possible endpoints, or just adding a function getEndpoints() in this interface? If we're just adding the function, when will we use that function?

Let's do that in a future iteration. Nothing pressing. I think with so few services, it'd be fairly self explanatory.

It's basically akin to doing a "help" function in any other program -- including bookie shell help command. Just to be able to see what all is available on the current implementation.

Make sense, maybe we can add a endpoint in the future to query the list of available http endpoints in the future.

dcastor · 2017-07-27T06:15:50Z

...p/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxAbstractHandler.java

+                return HttpServer.Method.DELETE;
+            case PUT:
+                return HttpServer.Method.PUT;
+            default:


Could just omit first case GET and let it default.

Good catch, fixed.

dcastor · 2017-07-27T06:44:51Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/http/ConfigurationService.java

+    protected ServerConfiguration conf;
+
+    public ConfigurationService(ServerConfiguration conf) {
+        assert conf != null;


Can we throw an exception instead?

I think we can consider using Preconditions.checkNull

Yup, fixed.

sijie · 2017-07-28T19:22:21Z

@eolivelli : @yzang already addressed your comments, can you review it again?

@dcastor : can you review @yzang 's comment? let's move this forward if possible.

eolivelli · 2017-07-29T16:09:27Z

Another comment. My bad I did not think about it before. What about security?
We are exposing bookie config and so disk layout, this usually is seen as some sort of information disclosure useful for attacks.
I think that at this moment it is not really relevant and we can go on, httpserver is optional.

Please give me some other day and I will be back with my +1 soon

eolivelli · 2017-07-30T19:45:41Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/http/JsonUtil.java

+public class JsonUtil {
+
+    public static String toJson(Object object) throws ParseJsonException {
+        ObjectMapper mapper = new ObjectMapper();


Object Mapper is threadsafe and usually it can be kept as a static field. It cachea the structure of classes and so it is more performant to keep only one instance please use only a static field

Make sense, will fix it

eolivelli · 2017-07-31T11:41:02Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

+    }
+
+    /**
+     * Get whether the http server start with auto-recovery service or not


comment is not correct at 100%, this options applied both to the bookie and to the auto-recovery daemon

Good catch, I'll fix it

eolivelli · 2017-07-31T11:49:09Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

+     *          Port to listen on
+     * @return server configuration
+     */
+    public ServerConfiguration setHttpServerPort(int port) {


@yzang @sijie
I wonder if we need to make the "port" configurable, it is be a "specific" value for each implementation

If you want to let the bookie "compose" the base URL of the service you will need protocol + hostname + port, and not just the port. We can add a method to the HttpServer interface to make the Bookie query the httpserver for the actual endpoint URL

Ideas:

make at least port/address/protocol (http/https) configurable

leave the 'port' parameter to each specific httpserver implementation (drop it from ServerConfiguration)

An example: in my case I will run my bookie inside a JVM which alredy bundles its own Tomcat based server and I will implement an HttpServlet. The "configuration" of the actual http endpoint will not be known to the Bookie at all.

In this case the httpserver port attribute will be ignored and I know that my case will work, but now that we are introducing this new feature a would like to have only the minimal set of configuration options.

Thoughts ?

@eolivelli
For the first point, I kinda agree with you, but I'm still thinking about what's the benefit of doing that, do you minding sharing some examples of under when will expose and make port/address/protocol configurable be beneficial?

For the second point, it is already achieved in this change, the interface in HttpServer allows youto pass the port in when you start the http server. The ServerConfiguration is just another way to make it possible to get the port from configuration, which could be useful in some other use cases. For example, in Twitter, we don't have a static port available for HttpServer, because we use aurora as the scheduler, the http port is dynamically assigned by aurora scheduler, so the only way to get the port is from configuration.

@eolivelli typically in a containerized environment, port is assigned by the scheduler. being able to configure a port is required, but I am not sure about protocol and base url, those two settings are somehow beyond the scope for bookkeeper, because the goal for this isn't to implement a generic http framework.

@sijie @yzang I apologize I was not clear in my comment.
I think that there are two opposite ways:

the implementation handles all of the parameters
so Vertx, TwitterServer impl can have their own "port" parameter, there will be no 'standard' http port configuration parameter

we define that there are the common 3 parameters for every httpserver implementation:

port (defaults to xxxx)

binding address (defaults to 0.0.0.0)

secure: true|false (default to false, maybe not implemented by vertx/twitter server)

maybe we can add 'secure' in a future time, but if we make configurable the port, let's add at least the 'binding address'.

if we make address/port/secure configurable with 'standard' attributes it will be easy to recompose the URLs of the API just by reading the configuration file

@eolivelli we can take the second approach and agree on it.

but regarding the binding address, can we defer this to a separate pull request? because it would be good to re-use the binding address the bookie service, rather than introducing new settings. let's try to introduce settings when we really need it.

@sijie OK, I the address of the bookie service is already accessible from other machines and I think it is a very convenient 'default'. Let's stick to it now.

So I am OK with this patch

eolivelli · 2017-07-31T11:53:03Z

...eeper-http/http-server/src/main/java/org/apache/bookkeeper/http/service/ServiceResponse.java

+        this.code = code;
+    }
+
+    public String getBody() {


I think it would be more future-proof not to return a java.lang.String.
IMHO It would be better to have these methods:

writeTo(OutputStream method) (or a getInputStream())

getContentType()

getContentLength()

If in the future maybe we will deal with large responses and storing them all in a a single String variable could be a problem

I think for the first version, maybe let's not make it that complicated? We currently don't expected very huge response (is larger than a single String can handle). We have an API in Twitter, which return all the ledgers that is underreplicated, and the largest response once return 300k ledgerIds, and it also works fine.

So I think for the current being, "String" is probably good enough, we can definitely extend the behavior in the future if we have more strong requirements. What do you think? @eolivelli

+1 for starting with simpler solution.

@yzang ok for the String version, it is simpler and will handle most of the first usecases

eolivelli · 2017-07-31T11:54:34Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/http/ConfigurationService.java

+        while (iterator.hasNext()) {
+            String key = iterator.next().toString();
+            List values = conf.getList(key);
+            if (values.size() > 0) {


lists of more than one value ?

We usually expect there's only one value for each configuration property, so I think the value size won't be greater than one here.

I don't think we will ever get value more than one.

@yzang want about using Object conf.getProperty(key) ?
in case of lists we will dump a list (which will be marshalled by the JSON encoder) and for every other kind of property we will get the same result as the current code by in a clearer way

That's a great idea, fixed in latest commit.

eolivelli

I have left some comments.
Overall is OK and I think it is a great feature
I would like to make some aspects clear, see the comments about the Response API and the 'port' configuration

sijie · 2017-08-01T06:19:31Z

@eolivelli the http port can be turned off by default. I don't see a strong point to include security at this point. we can come up with security supported in subsequent pull requests.

eolivelli · 2017-08-01T06:31:49Z

@sijie for the "security". Yes I think it is not required and this time as httpserver will be turned off by default

eolivelli

+1 LGTM I think this is a great feature for BK ! Thank you @yzang @sijie

…behavior, and add basic http server test cases

sijie · 2017-08-14T18:08:35Z

@eolivelli @dcastor just to check if we are good to go with this pull request.

eolivelli · 2017-08-14T19:56:25Z

Yes I am ok. Thank you for asking my ack.
+1

Add Bookkeeper Http Server as a new module [bookkeeper-http] 1. Add children module [http-server], which provide generic api for setting up a Http Server for bookkeeper, allowing to plugin different Http frameworks and inject the implementation of different BK related services to Http Handler to handle different endpoints 2. Add children module [vertx-http-server], which is a Vertx.io based http server implementation 3. Add children module [twitter-http-server], which is a TwitterServer based http server implementation 4. Add a package or org.apache.bookkeeper.http in [bookkeeper-server], which include all the implementation for the services that handle different http endpoint request. Author: Yiming Zang <yzang@twitter.com> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This patch had conflicts when merged, resolved by Committer: Sijie Guo <sijie@apache.org> This closes apache#278 from yzang/yzang/BOOKKEEPER-1100

Descriptions of the changes in this PR: #278 introduces BookKeeper Http Endpoint module. However there are only two endpoints, which is “/heartbeat” and “/api/config/serverConfig”. In order to fully leverage the http modules, The goal is to add more endpoints to this modules. Please reference BP-17 for more details. Author: Jia Zhai <zhaijia@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes #521 from zhaijack/http_endpoint, closes #520

Yiming Zang added 4 commits June 24, 2017 21:10

Move Http Server to a separate module [bookkeeper-http],

fd159b2

[http-server] is a child module which provide generic http server, http handlers and http service APIs. [vertx-http-server] and [twitter-http-server] are two implementations of http server.

Merge branch 'apache_mirror_local' into yzang/BOOKKEEPER-1100

0f6c67f

Do not enable http by default

0ee0b18

sijie approved these changes Jul 24, 2017

View reviewed changes

Fix style issues

4be4f70

eolivelli requested changes Jul 25, 2017

View reviewed changes

Yiming Zang added 2 commits July 25, 2017 23:58

Use jackson for json serializaiton and fix some styles

aa43eb1

Add a builder for BKServiceProvider

6a450a5

dcastor reviewed Jul 27, 2017

View reviewed changes

Use Precondition to check null and improve switch case

6ef9eda

eolivelli reviewed Jul 30, 2017

View reviewed changes

eolivelli reviewed Jul 31, 2017

View reviewed changes

eolivelli requested changes Jul 31, 2017

View reviewed changes

eolivelli approved these changes Aug 1, 2017

View reviewed changes

sijie added this to the 4.6.0 milestone Aug 1, 2017

Yiming Zang added 2 commits August 1, 2017 17:39

Move JsonUtil to util package, add HttpRouter to abstract the router …

1cfae71

…behavior, and add basic http server test cases

Move JsonUtil to util package, add HttpRouter to abstract the router …

671450e

…behavior, and add basic http server test cases

Yiming Zang added 2 commits August 1, 2017 18:20

Merge conflict for BOOKKEEPER-1100 and address new checkstyle issues

88747b0

Address more merge conflicts

f0f3455

eolivelli approved these changes Aug 14, 2017

View reviewed changes

sijie mentioned this pull request Aug 15, 2017

Document Http Endpoint #449

Closed

sijie added area/bookie release/4.6.0 type/feature labels Aug 16, 2017

sijie assigned yzang Aug 16, 2017

sijie closed this in de9eecc Aug 16, 2017

This was referenced Sep 24, 2017

Add more http endpoint based on current http endpoint module #520

Closed

Issue 520: Add more http endpoint #521

Closed

sijie mentioned this pull request Nov 9, 2017

Documentation for HTTP endpoints #674

Closed

BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint #278

BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint #278

Conversation

yzang commented Jul 24, 2017 • edited by sijie Loading

sijie left a comment

Choose a reason for hiding this comment

sijie left a comment

Choose a reason for hiding this comment

yzang commented Jul 25, 2017 • edited Loading

eolivelli left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijie commented Jul 25, 2017

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

dcastor Jul 27, 2017 • edited Loading

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

sijie commented Jul 28, 2017

eolivelli commented Jul 29, 2017

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

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

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

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

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

sijie commented Aug 1, 2017

eolivelli commented Aug 1, 2017

eolivelli left a comment

Choose a reason for hiding this comment

sijie commented Aug 14, 2017

eolivelli commented Aug 14, 2017

yzang commented Jul 24, 2017 •

edited by sijie

Loading

yzang commented Jul 25, 2017 •

edited

Loading

dcastor Jul 27, 2017 •

edited

Loading