Issue 674: Documentation for HTTP endpoints #721

zhaijack · 2017-11-13T11:21:01Z

Descriptions of the changes in this PR:
Add a document for http endpoints.

ivankelly · 2017-11-13T11:31:44Z

site/docs/latest/admin/http.md

+title: BookKeeper http endpoints
+---
+
+This document is to introducing BookKeeper http endpoints that could be used for BookKeeper administration.


This document introduces BookKeeper HTTP endpoints, which can be used ...

Thanks, will change it.

ivankelly · 2017-11-13T11:32:46Z

site/docs/latest/admin/http.md

+---
+
+This document is to introducing BookKeeper http endpoints that could be used for BookKeeper administration.
+If user want to use this feature, parameter "httpServerEnabled" need to be set to "true" in file conf/bk_server.conf.


To use this feature, set "httpServerEnabled" to "true" in file conf/bk_server.conf.

Thanks, wll change it.

ivankelly · 2017-11-13T11:35:17Z

site/docs/latest/admin/http.md

+
+## All the endpoints
+
+Currently all the http endpoints could be divided into these 4 components:


The HTTP endpoints are divided into the following groups:

Perhaps they should be grouped by endpoints which are bookie specific and those which are for the whole cluster.

Thanks. will change the words.
And it is a good idea to do a new grouping following your suggestion. will consider to do it, and opened a new issue #722 to tracking this, but as the requirements goes on, maybe 2 big groups need futher dividing.

ivankelly · 2017-11-13T11:41:42Z

site/docs/latest/admin/http.md

+        |200 | Successful operation |
+        |403 | Don't have permission |
+        |404 | Error handling |
+1. Method: PUT


Is there already an admin command to do this? Does this access a config in zookeeper or on the local disk?

seems not command for this. it only update the inmemory value, which could be used for later ops.

I'm not sure it's wise to expose functionality like this at all. It's just asking for people to change configuration in an uncontrolled and unpredictable manner. Was there a driving use case to add this to the http endpoints?

@ivankelly I think http endpoints (and configuration service) was introduced by twitter initially and Jia extended it to cover all admin commands. I can see this is useful on dynamic configurations. but Twitter folks can chime in more on the use cases.

BTW If there is a question on a feature whether this should be included or not, we should probably be discussing a bit earlier on the coding pull request, rather than later on the documentation pull request. (for future improvements)

ivankelly · 2017-11-13T11:42:34Z

site/docs/latest/admin/http.md

+
+        | Name | Type | Required | Description |
+        |:-----|:-----|:---------|:------------|
+        |metadata  | Boolean | No |  whether print out metadata  |


api doesn't print.

ivankelly · 2017-11-13T11:43:56Z

site/docs/latest/admin/http.md

+        |404 | Error handling |
+
+### Endpoint: /api/v1/ledger/read/?ledger_id=<ledger_id>&start_entry_id=<start_entry_id>&end_entry_id=<end_entry_id>
+1. Method: GET


What format are these entries returned in?

thanks. will add the format

ivankelly · 2017-11-13T11:44:27Z

site/docs/latest/admin/http.md

+
+## Config
+
+### Endpoint: /api/v1/config/server_config


The format of the response should be included in each call.

Thanks. the response is construct of code and body, and some of the command the response body not contains too much info. Will add the useful respone body format.

ivankelly · 2017-11-13T11:45:32Z

site/docs/latest/admin/http.md

+
+        | Name | Type | Required | Description |
+        |:-----|:-----|:---------|:------------|
+        |bookie_src  | String | Yes | bookie source to recovery |


String or array of strings?

Should the strings have a specific format?

strings of host names or IP:Port

@zhaijack with my latest change in #612, it should be a list of strings.

ivankelly · 2017-11-13T11:46:46Z

site/docs/latest/admin/http.md

+
+        | Name | Type | Required | Description |
+        |:-----|:-----|:---------|:------------|
+        |missingreplica  | String | No | missing replica bookieId |


These parameters should have underscores between words to be consistent.

@ivankelly I think this is a documentation for what existed. if the parameters should be changed, we should create an issue for it.

ivankelly · 2017-11-13T11:47:30Z

site/docs/latest/admin/http.md

+
+        | Name | Type | Required | Description |
+        |:-----|:-----|:---------|:------------|
+        |type      | String | Yes |  value: rw/ro , list read-write/read-only bookies. |


rw/ro is the value? or rw or ro?

will change to "rw or ro"

asfgit · 2017-11-13T12:22:27Z

FAILURE

--none--

sijie · 2017-11-13T19:16:49Z

/cc @lucperkins for documentation reviews.

lucperkins · 2017-11-13T20:35:27Z

For the sake of maintainability, I think it'd be best to generate these docs from a JSON or YAML definition using a template, but I can do that work in a follow-up PR

sijie · 2017-11-13T22:42:28Z

@lucperkins a nice point! +1

sijie · 2017-11-16T19:14:43Z

site/_data/sidebar.yaml

@@ -28,6 +28,8 @@ groups:
    endpoint: metrics
  - name: Upgrade
    endpoint: upgrade
+  - name: BookKeeper http endpoint


I would suggest changing "http endpoint" to "Admin REST API", which would be more accurate.

thanks. will change it.

sijie · 2017-11-16T19:16:49Z

site/docs/latest/admin/http.md

@@ -0,0 +1,394 @@
+---
+title: BookKeeper http endpoints


I would suggest changing "http endpoint" to "Admin REST API", which would be more accurate.

thanks. will change it.

lucperkins · 2017-11-13T20:00:24Z

site/_data/sidebar.yaml

@@ -28,6 +28,8 @@ groups:
    endpoint: metrics
  - name: Upgrade
    endpoint: upgrade
+  - name: BookKeeper http endpoint


Capitalize HTTP

“endpoints” instead of “endpoint”

thanks. will follow @sijie 's comments

lucperkins · 2017-11-13T20:01:34Z

site/docs/latest/admin/http.md

+---
+
+This document introduces BookKeeper HTTP endpoints, which can be used for BookKeeper administration.
+To use this feature, set "httpServerEnabled" to "true" in file conf/bk_server.conf.


All variable and file names should be in backticks rather than quotation marks

Thanks. will change it.

lucperkins · 2017-11-13T20:01:48Z

site/docs/latest/admin/http.md

+
+## All the endpoints
+
+Currently all the http endpoints could be divided into these 4 components:


HTTP should be capitalized when used in sentences

Thanks. will change it.

asfgit · 2017-11-17T05:31:58Z

FAILURE

--none--

Descriptions of the changes in this PR: Add a document for http endpoints. Author: Jia Zhai <zhaijia@apache.org> Reviewers: Luc Perkins <lucperkins@gmail.com>, Sijie Guo <sijie@apache.org> This closes #721 from zhaijack/http_doc, closes #674 (cherry picked from commit c8c3f67) Signed-off-by: Jia Zhai <zhaijia@apache.org>

Descriptions of the changes in this PR: Add a document for http endpoints. Author: Jia Zhai <zhaijia@apache.org> Reviewers: Luc Perkins <lucperkins@gmail.com>, Sijie Guo <sijie@apache.org> This closes apache#721 from zhaijack/http_doc, closes apache#674

add http doc

eacda51

ivankelly reviewed Nov 13, 2017

View reviewed changes

change following comments

5b3adf8

sijie reviewed Nov 16, 2017

View reviewed changes

lucperkins approved these changes Nov 16, 2017

View reviewed changes

change following comments in codereview

64e2919

jiazhai added this to the 4.7.0 milestone Nov 18, 2017

jiazhai added area/documentation release/4.7.0 type/task labels Nov 18, 2017

jiazhai assigned zhaijack Nov 18, 2017

jiazhai closed this in c8c3f67 Nov 18, 2017

jiazhai added the release/4.6.0 label Nov 18, 2017

sijie removed the release/4.7.0 label Nov 22, 2017

sijie modified the milestones: 4.7.0, 4.6.0 Nov 22, 2017


		## All the endpoints

		Currently all the http endpoints could be divided into these 4 components:

Issue 674: Documentation for HTTP endpoints #721

Issue 674: Documentation for HTTP endpoints #721

Conversation

zhaijack commented Nov 13, 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

zhaijack Nov 13, 2017 • 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

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

asfgit commented Nov 13, 2017

sijie commented Nov 13, 2017

lucperkins commented Nov 13, 2017

sijie commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiazhai Nov 17, 2017 • 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

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

asfgit commented Nov 17, 2017

zhaijack Nov 13, 2017 •

edited

jiazhai Nov 17, 2017 •

edited