Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/3386 )
Change subject: [ASTERIXDB-2597] Load UDFs via HTTP ...................................................................... Patch Set 9: (21 comments) https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@64 PS9, Line 64: if (path.length != 5) { why 5? In the document you only have 4 parts: "admin/udf/fooverse/mylib" https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@78 PS9, Line 78: return; I'd move response.setStatus(HttpResponseStatus.BAD_REQUEST) here from getResource(). getResource() could then return 'null' if it couldn't get resource name from the request. Then this method will control HTTP response status. https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@84 PS9, Line 84: File udf = File.createTempFile(resourceName, ".zip"); Should we have 'temp' directory under the "base" directory (ServerContext.getBaseDir())? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@88 PS9, Line 88: ByteBuffer content = request.getHttpRequest().content().nioBuffer(); minor. you're already getting req.content() in line 86 above. can we assign it to a variable there and just use this variable here? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@96 PS9, Line 96: ClassLoader cl = appCtx.getLibraryManager().getLibraryClassLoader(dataverse, resourceName); related question: what's the parent of the library classloader? I think we need to set it up, so the deployed code does not see hyracks classes. May be let's discuss it later https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@109 PS9, Line 109: broker.sendSyncRequestToNCs(reqId, ncs, requests, 5000); minor. let's create a constant for this timeout. https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@111 PS9, Line 111: response.setStatus(HttpResponseStatus.SERVICE_UNAVAILABLE); Should we return INTERNAL_SERVER_ERROR instead? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@114 PS9, Line 114: } finally { when are we deleting 'udf' temp file? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@132 PS9, Line 132: appCtx.getLibraryManager().deregisterLibraryClassLoader(dataverse, resourceName); where are we closing the classloader? I think we need to close it *(URLClassLoader.close()) before attempting to delete the jar files that are opened by it. https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@147 PS9, Line 147: response.setStatus(HttpResponseStatus.BAD_REQUEST); Should we return INTERNAL_SERVER_ERROR instead? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/DeleteUdfMessage.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/DeleteUdfMessage.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/DeleteUdfMessage.java@30 PS9, Line 30: private final String deploymentId; why can't we have 2 strings: dataverseName, libraryName? so we don't have to concat / split? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/LoadUdfMessage.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/LoadUdfMessage.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/LoadUdfMessage.java@33 PS9, Line 33: private final String deploymentId; why not have two strings? dataverseName, libraryName? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java@202 PS9, Line 202: // librarian.cleanup(); minor. if this line is not needed anymore, then let's remove it https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java@154 PS9, Line 154: // ExternalUDFLibrarian.removeLibraryDir(); minor. if this line is not needed anymore, then let's remove it https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/bootstrap/TestNodeController.java@169 PS9, Line 169: // ExternalUDFLibrarian.removeLibraryDir(); minor. if this line is not needed anymore, then let's remove it https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/LangExecutionUtil.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/LangExecutionUtil.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/LangExecutionUtil.java@86 PS9, Line 86: // ExternalUDFLibrarian.removeLibraryDir(); minor. if this line is not needed anymore then let's remove it https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@106 PS9, Line 106: String[] fileSplits = urls.get(0).toString().split("\\."); minor. use getPath() instead of toString() https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@108 PS9, Line 108: if (urls.size() == 1 && "zip".equalsIgnoreCase(extension)) { I think this slightly changes the semantics of this method by adding special handling for a case when there's one URL and it's a zip file. Can we have caller decide what needs to be done? May be have two method parameters: extractFromArchive and addToClassPath? so the caller can specify what should happen with these urls instead of us trying to figure out it here based on the number of files and a file extension? https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@187 PS9, Line 187: private static List<URL> downloadURLs(List<URL> urls, String deploymentDir, boolean isNC, boolean isZip) very minor I'd rename isZip to extractFromArchive, to give a clear indication of what is supposed to happen. https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@227 PS9, Line 227: e.printStackTrace(); > CRITICAL SonarQube violation: minor. printStackTrace() needs to be removed (logging exception if necessary) https://asterix-gerrit.ics.uci.edu/#/c/3386/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@234 PS9, Line 234: public static void unzip(String sourceFile, String outputDir) throws IOException { just curious, what would happen with these files if let's say the zip file is corrupt and couldn't be fully extracted. Will they be cleaned up at some point be some other code? -- To view, visit https://asterix-gerrit.ics.uci.edu/3386 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6be9fef54c010bdb32f5c78af9b973f9843f442f Gerrit-Change-Number: 3386 Gerrit-PatchSet: 9 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose (1000171) Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Fri, 21 Jun 2019 23:32:17 +0000 Gerrit-HasComments: Yes
