You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/02/08 12:09:35 UTC
[GitHub] [ignite-3] rpuch commented on a change in pull request #640: IGNITE-16495 Make RestModule handlers asynchronous
rpuch commented on a change in pull request #640:
URL: https://github.com/apache/ignite-3/pull/640#discussion_r801545844
##########
File path: modules/rest/src/main/java/org/apache/ignite/internal/rest/RestModule.java
##########
@@ -232,34 +244,40 @@ private void handleRepresentByPath(
* @param res Rest response.
* @param presentation Configuration presentation.
*/
- private void handleUpdate(
+ private static CompletableFuture<RestApiHttpResponse> handleUpdate(
RestApiHttpRequest req,
RestApiHttpResponse res,
ConfigurationPresentation<String> presentation
) {
- try {
- String updateReq = req
- .request()
- .content()
- .readCharSequence(req.request().content().readableBytes(), UTF_8)
- .toString();
-
- presentation.update(updateReq);
- } catch (IllegalArgumentException e) {
- ErrorResult errRes = new ErrorResult("INVALID_CONFIG_FORMAT", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- } catch (ConfigurationValidationException e) {
- ErrorResult errRes = new ErrorResult("VALIDATION_EXCEPTION", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- } catch (IgniteException e) {
- ErrorResult errRes = new ErrorResult("APPLICATION_EXCEPTION", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- }
+ String updateReq = req
+ .request()
+ .content()
+ .readCharSequence(req.request().content().readableBytes(), UTF_8)
Review comment:
This seems to read the request body, so this probably means some I/O and (for large requests) it might take a lot of time. Would it make sense to read the request body asynchronously?
##########
File path: modules/rest/src/main/java/org/apache/ignite/internal/rest/netty/RestApiHandler.java
##########
@@ -63,55 +63,63 @@ public RestApiHandler(Router router) {
/** {@inheritDoc} */
@Override
- public void channelReadComplete(ChannelHandlerContext ctx) {
- ctx.flush();
- }
+ protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) {
+ CompletableFuture<DefaultFullHttpResponse> responseFuture = router.route(request)
+ .map(route -> {
+ var response = new RestApiHttpResponse(new DefaultHttpResponse(HttpVersion.HTTP_1_1, OK));
- /** {@inheritDoc} */
- @Override
- protected void channelRead0(ChannelHandlerContext ctx, HttpObject msg) throws Exception {
- if (msg instanceof FullHttpRequest) {
- FullHttpRequest req = (FullHttpRequest) msg;
- FullHttpResponse res;
-
- var maybeRoute = router.route(req);
- if (maybeRoute.isPresent()) {
- var resp = new RestApiHttpResponse(new DefaultHttpResponse(HttpVersion.HTTP_1_1, OK));
- maybeRoute.get().handle(req, resp);
- var content = resp.content() != null
- ? Unpooled.wrappedBuffer(resp.content()) : new EmptyByteBuf(UnpooledByteBufAllocator.DEFAULT);
- res = new DefaultFullHttpResponse(resp.protocolVersion(), resp.status(),
- content, resp.headers(), EmptyHttpHeaders.INSTANCE);
- } else {
- res = new DefaultFullHttpResponse(req.protocolVersion(), HttpResponseStatus.NOT_FOUND);
- }
-
- res.headers()
- .setInt(CONTENT_LENGTH, res.content().readableBytes());
-
- boolean keepAlive = HttpUtil.isKeepAlive(req);
- if (keepAlive) {
- if (!req.protocolVersion().isKeepAliveDefault()) {
- res.headers().set(CONNECTION, KEEP_ALIVE);
- }
- } else {
- res.headers().set(CONNECTION, CLOSE);
- }
-
- ChannelFuture f = ctx.write(res);
-
- if (!keepAlive) {
- f.addListener(ChannelFutureListener.CLOSE);
- }
- }
+ return route.handle(request, response)
+ .thenApply(resp -> {
+ ByteBuf content = resp.content() != null
+ ? Unpooled.wrappedBuffer(resp.content())
+ : new EmptyByteBuf(UnpooledByteBufAllocator.DEFAULT);
+
+ return new DefaultFullHttpResponse(
+ resp.protocolVersion(),
+ resp.status(),
+ content,
+ resp.headers(),
+ EmptyHttpHeaders.INSTANCE
+ );
+ });
+ })
+ .orElseGet(() -> CompletableFuture.completedFuture(
+ new DefaultFullHttpResponse(request.protocolVersion(), HttpResponseStatus.NOT_FOUND)
+ ));
+
+ responseFuture
+ .whenCompleteAsync((response, e) -> {
+ if (e != null) {
+ exceptionCaught(ctx, e);
+
+ return;
+ }
+
+ response.headers().setInt(CONTENT_LENGTH, response.content().readableBytes());
Review comment:
Will `readableBytes()` always be equal to the full content length?
##########
File path: modules/rest/src/main/java/org/apache/ignite/internal/rest/RestModule.java
##########
@@ -232,34 +244,40 @@ private void handleRepresentByPath(
* @param res Rest response.
* @param presentation Configuration presentation.
*/
- private void handleUpdate(
+ private static CompletableFuture<RestApiHttpResponse> handleUpdate(
RestApiHttpRequest req,
RestApiHttpResponse res,
ConfigurationPresentation<String> presentation
) {
- try {
- String updateReq = req
- .request()
- .content()
- .readCharSequence(req.request().content().readableBytes(), UTF_8)
- .toString();
-
- presentation.update(updateReq);
- } catch (IllegalArgumentException e) {
- ErrorResult errRes = new ErrorResult("INVALID_CONFIG_FORMAT", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- } catch (ConfigurationValidationException e) {
- ErrorResult errRes = new ErrorResult("VALIDATION_EXCEPTION", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- } catch (IgniteException e) {
- ErrorResult errRes = new ErrorResult("APPLICATION_EXCEPTION", e.getMessage());
-
- res.status(BAD_REQUEST);
- res.json(Map.of("error", errRes));
- }
+ String updateReq = req
+ .request()
+ .content()
+ .readCharSequence(req.request().content().readableBytes(), UTF_8)
+ .toString();
+
+ return presentation.update(updateReq)
+ .thenApply(v -> res)
Review comment:
If everything is ok, do we return an empty response with code 200, or just an empty json `{}` ? It's not what you are fixing here, but I'm just curious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org