abdullah alamoudi has posted comments on this change. Change subject: Replace Servlets with Netty Based HTTP Servers ......................................................................
Patch Set 1: (41 comments) https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiLet.java: Line 89: } catch (IllegalArgumentException e) { > CRITICAL SonarQube violation: Done Line 103: ILetResponse.setCharacterEncoding(response, ILetResponse.TEXT_HTML, ILetResponse.ENCODING_UTF8); > change this method name to setContentType Done Line 109: hds = (IHyracksDataset) map.get(HYRACKS_DATASET_ATTR); > This is not thread safe, this can return a partially initialized IHyracksDa map is concurrent. this should be fine I think. Line 131: double duration = 0; > MAJOR SonarQube violation: Done Line 143: ResultUtil.webUIErrorHandler(response.writer(), e); > reintroduce "out" variable? Done Line 147: public void doGet(ILetRequest request, ILetResponse response) throws Exception { > CRITICAL SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ChunkedNettyOutputStream.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ChunkedNettyOutputStream.java: Line 91: } else{ > MAJOR SonarQube violation: Done Line 91: } else{ > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ClusterApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ClusterApiLet.java: Line 128: requestURL.append(request.getHeader(HttpHeaderNames.HOST.toString())); > Update getHeader(String) -> getHeader(CharSequence)? Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ClusterControllerDetailsApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ClusterControllerDetailsApiLet.java: Line 51: if (path(request).equals("")) { > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ConnectorApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ConnectorApiLet.java: Line 45: public class ConnectorApiLet extends AbstractLet { > Bring back the class comment. Done Line 51: private void formResponseObject(ObjectNode jsonResponse, FileSplit[] fileSplits, ARecordType recordType, > Move this method down to reduce diff size. Done Line 52: String primaryKeys, boolean temp, Map<String, NodeControllerInfo> nodeMap) throws Exception { > CRITICAL SonarQube violation: Done Line 63: String ipAddress = nodeMap.get(split.getNodeName()).getNetworkAddress().getAddress().toString(); > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FeedLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FeedLet.java: Line 44: if (requestURI.equals("/")) { > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FullLetResponse.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FullLetResponse.java: Line 64: String txt = new String(baos.toByteArray(), StandardCharsets.UTF_8); > MAJOR SonarQube violation: Done Line 64: String txt = new String(baos.toByteArray(), StandardCharsets.UTF_8); > unused. Done Line 64: String txt = new String(baos.toByteArray(), StandardCharsets.UTF_8); > MAJOR SonarQube violation: Done Line 66: fullResponse.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, fullResponse.content().readableBytes()); > Check if the HTTP spec is ok not having a length? Or does netty set it? Either set length or send an empty response chunk indicating end. since this is a full response and we know the size, it is better to just set it. https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/GetLetRequest.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/GetLetRequest.java: Line 42: List<String> parameter = parameters.get(name); > Share code with PostLetRequest. Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServer.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServer.java: Line 73: Collections.sort(lets, (l1, l2) -> l2.getPaths()[0].length() - l1.getPaths()[0].length()); > Does this work? Is the goal to make sure that we access handlers in the rig This is a hacky way to ensure that ILets with more specific paths are checked first. For example, /path/to/resource/ is checked before /path/to/ before /path/ https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerHandler.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerHandler.java: Line 69: HttpResponseStatus.NOT_FOUND); > Should this be "method not alowed"? Done Line 76: let.handle(request, response); > Should we just catch Throwable here and set the response code to 500? That Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILet.java: Line 23: public interface ILet { > Why Let? Seems a little too nondescript. Can we stick with Servlet, or us Done Line 28: void handle(ILetRequest request, ILetResponse response) throws Exception; > Can we have a specific or no exception here? Would that make sense? It does. changing to no exception https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILetResponse.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILetResponse.java: Line 36: public static final String ENCODING_UTF8 = "utf-8"; > Pull those out into a ContentType and an Encoding? Maybe netty has those al Done Line 54: String getHeader(CharSequence name) throws IOException; > Why do we need to get headers? I think that we remove it. Done Line 80: OutputStream outputStream(); > Do we need an OutputStream and a Writer? If we keep them, could be put the Done Line 82: public static void setCharacterEncoding(ILetResponse response, String type, String charset) throws IOException { > rename to setContentType Done Line 83: response.addHeader(HttpHeaderNames.CONTENT_TYPE, type + "; charset=" + charset); > It seems this won't do the right thing if setCharacterEndoding/setContentTy Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryStatusApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryStatusApiLet.java: Line 60: hds = (IHyracksDataset) map.get(HYRACKS_DATASET_ATTR); > This is not thread safe, this can return a partially initialized IHyracksDa I am pretty sure this is safe since map is a concurrent map https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryWebInterfaceLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryWebInterfaceLet.java: Line 31: import org.apache.commons.logging.LogFactory; > Use a different logging framework? Done Line 57: public void doGet(ILetRequest request, ILetResponse response) throws IOException { > Should this be protected or private? Done Line 97: public void doPost(ILetRequest request, ILetResponse response) throws IOException { > Should this be protected or private? yes Line 111: response.setStatus(HttpResponseStatus.INTERNAL_SERVER_ERROR); > This could be handled in the framework? Done Line 122: public static String mime(String extension) { > Pull this out into a ContentType utility? Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiLet.java: Line 87: if (output.equals("CSV")) { > MAJOR SonarQube violation: Done Line 89: } else if (output.equals("ADM")) { > MAJOR SonarQube violation: Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UpdateApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UpdateApiLet.java: Line 36: protected String getErrorMessage() { > Move this down? Done https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java: Line 86: private List<HttpServer> nettyServers; > Would it make sense to encapsulate the Netty boilerplate? Done Line 222: protected ILet createLet(HttpServer server, Lets key, String... paths) { > Move this down? Done -- To view, visit https://asterix-gerrit.ics.uci.edu/1429 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3d552d4eb8c868535ca4c41cbcf7e352217b18ae Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
