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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
