Adds the bookie cookie service for http api #4541
Conversation
|
@zymap @eolivelli @hangc0276 @horizonzy please help review this pr, thanks. |
|
seems like it is working for BP-68 |
Yes, there are two http apis. But both are used to delete the decommissioned bookie's cookie information. BP-68 will delete cookie as part of decommission API. This API("/api/v1/bookie/cookie") is invoked by the operation and maintenance platform to delete cookies after detecting that the bookie is decommissioned. |
StevenLuMT
requested review from
hangc0276,
eolivelli,
hezhangjian,
zymap and
StevenLuMT
| case GET: | ||
| Versioned<Cookie> cookie = Cookie.readFromRegistrationManager(registrationManager, bookieId); | ||
| return new HttpServiceResponse(cookie.toString(), HttpServer.StatusCode.OK); | ||
| case DELETE: |
There was a problem hiding this comment.
There is nothing wrong with adding new interfaces,
but I think deleting cookies is a bit risky and does not comply with data integrity specifications. Although this is available in the old Command command, I suggest you still go through the node offline process ./bin/bookkeeper shell decommissionbookie [-bookieid <bookieaddress>]
There was a problem hiding this comment.
Agreed, the caller should call this api only after bookie decommission is completed.
| case GET: | ||
| Versioned<Cookie> cookie = Cookie.readFromRegistrationManager(registrationManager, bookieId); | ||
| return new HttpServiceResponse(cookie.toString(), HttpServer.StatusCode.OK); | ||
| case DELETE: |
There was a problem hiding this comment.
Why not add a method to add a cookie while we have a method to delete a cookie.
SongOf
force-pushed
the
cookie_http_api
branch
from
6bf61a7 to
c1db27b
Compare
|
ci failed,please check @SongOf |
SongOf
force-pushed
the
cookie_http_api
branch
from
c1db27b to
694a9c9
Compare
There was a problem hiding this comment.
Deleting a cookie without validation that decomm is done can be an expensive operator error. I think the command must check for that (as with node decommission, make sure node is offline). So I fully support this concern https://github.com/apache/bookkeeper/pull/4541/files#r1955442058 and disagree with idea of making it an operator's problem.
Alternatively, we can expose recover command as a sequence of
- switch target node into the read-only mode
- run recover command
- run recover command (double-check) with the cookie delete option
the gotcha here is that only one recover must run at a time in my experience, but I think the same is valid for decomm command.
my user case: |
|
I think this may be an abnormal case of decommission, but you want to open the cookie interface now, which is not a good way. Covering up the error will only cover up the problem. It is better to check why decommission did not clean up the cookies and solve the problem from the source. |
|
I suggest temporarily closing this PR and reopening it later if necessary. |
Descriptions of the changes in this PR:
Motivation
It would be convenient to support get, or delete the cookie of bookie with REST API, just like what does in the shell command.
Changes
Adds an API to get, delete the cookie of bookie at /api/v1/bookie/cookie.