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

Fix trigger GC not work #3998

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

hangc0276
Copy link
Contributor

Motivation

When I use curl -XPUT http://<ip>:<port>/api/v1/bookie/gc command to trigger garbage compaction, the bookie server returns Internal Server Error. Then I checked the bookie server's log, and nothing was found.

After a deep debugging of the server, I found the following exception.

    org.apache.bookkeeper.common.util.JsonUtil$ParseJsonException: Failed to deserialize Object from Json string
	at org.apache.bookkeeper.common.util.JsonUtil.fromJson(JsonUtil.java:41)
	at org.apache.bookkeeper.server.http.service.TriggerGCService.handle(TriggerGCService.java:71)
	at org.apache.bookkeeper.http.vertx.VertxAbstractHandler.processRequest(VertxAbstractHandler.java:55)
	at org.apache.bookkeeper.http.vertx.VertxHttpHandlerFactory$1.handle(VertxHttpHandlerFactory.java:47)
	at org.apache.bookkeeper.http.vertx.VertxHttpHandlerFactory$1.handle(VertxHttpHandlerFactory.java:43)
	at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1038)
	at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:137)
	at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:132)
	at io.vertx.ext.web.handler.impl.BodyHandlerImpl$BHandler.doEnd(BodyHandlerImpl.java:296)
	at io.vertx.ext.web.handler.impl.BodyHandlerImpl$BHandler.end(BodyHandlerImpl.java:276)
	at io.vertx.ext.web.handler.impl.BodyHandlerImpl.lambda$handle$0(BodyHandlerImpl.java:87)
	at io.vertx.core.http.impl.HttpServerRequestImpl.onEnd(HttpServerRequestImpl.java:525)
	at io.vertx.core.http.impl.HttpServerRequestImpl.handleEnd(HttpServerRequestImpl.java:511)
	at io.vertx.core.http.impl.Http1xServerConnection.handleEnd(Http1xServerConnection.java:176)
	at io.vertx.core.http.impl.Http1xServerConnection.handleMessage(Http1xServerConnection.java:138)
	at io.vertx.core.impl.ContextImpl.executeTask(ContextImpl.java:366)
	at io.vertx.core.impl.EventLoopContext.execute(EventLoopContext.java:43)

This bug is introduced by #3205. When we use curl -XPUT http://<ip>:<port>/api/v1/bookie/gc command to trigger gc, the request body is empty string, but not null.

if (null == requestBody) {
bookieServer.getBookie().getLedgerStorage().forceGC();
} else {
@SuppressWarnings("unchecked")
Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class);
Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null);
Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null);
bookieServer.getBookie().getLedgerStorage().forceGC(forceMajor, forceMinor);
}

It will enter Line#68 but goes into Line#71. It uses JsonUtil.fromJson to parse an empty string will throw the above exception and will return 500 to the client but no logs in the server side.

Changes

  • Fix the request body check bug
  • Add a unit test to cover this logic

Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null);
Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null);
bookieServer.getBookie().getLedgerStorage().forceGC(forceMajor, forceMinor);
response.setCode(HttpServer.StatusCode.NOT_FOUND);
Copy link
Member

Choose a reason for hiding this comment

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

The status should be NOT_ALLOWED, see here https://httpwg.org/specs/rfc9110.html#status.405

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. It will change the current status code.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null);
Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null);
bookieServer.getBookie().getLedgerStorage().forceGC(forceMajor, forceMinor);
response.setCode(HttpServer.StatusCode.NOT_FOUND);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@zymap zymap merged commit 147b5bf into apache:master Jun 26, 2023
16 checks passed
zymap pushed a commit that referenced this pull request Jun 26, 2023
* Fix trigger gc not work

(cherry picked from commit 147b5bf)
zymap pushed a commit that referenced this pull request Dec 7, 2023
* Fix trigger gc not work

(cherry picked from commit 147b5bf)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
* Fix trigger gc not work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants