Michael Blow has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/3386 )
Change subject: [ASTERIXDB-2597] Load UDFs via HTTP ...................................................................... Patch Set 12: (14 comments) https://asterix-gerrit.ics.uci.edu/#/c/3386/12/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/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@56 PS12, Line 56: public final String UDF_TMP_DIR_PREFIX = "udf_temp"; > MAJOR SonarQube violation: +1 https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@57 PS12, Line 57: public final int UDF_RESPONSE_TIMEOUT = 5000; > MAJOR SonarQube violation: +1 https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@96 PS12, Line 96: raf = new RandomAccessFile(udf, "rw"); > BLOCKER SonarQube violation: can we wrap this in a try w/ resources, then you can remove the fc.close() below I think... https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@153 PS12, Line 153: return; should this be a bad request? https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UdfApiServlet.java@153 PS12, Line 153: return; oh, i see this is done above- can we set it here so that we set the statuses all in one method? (we set OK and 500 below). Should we just throw IllegalArgumentException or something above and insert a catch clause below? https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java@404 PS12, Line 404: StringBuilder logMesg = new StringBuilder("Classpath for library " + dataverse + "\n"); : for (URL url : urls) { : logMesg.append(url.getFile() + "\n"); : } : LOGGER.info(logMesg.toString()); multi line log entries complicates log post-processing, can we just create a list of files and emit a single-line for this? https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java@53 PS12, Line 53: http://"; + host + ":" + port i wonder if there are IPv6 implications here https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java@66 PS12, Line 66: http://"; + host + ":" + port same as above https://asterix-gerrit.ics.uci.edu/#/c/3386/12/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/12/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/LoadUdfMessage.java@59 PS12, Line 59: } catch (Exception f) { > CRITICAL SonarQube violation: +1 https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/library/ILibraryManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/library/ILibraryManager.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/12/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/library/ILibraryManager.java@33 PS12, Line 33: rm https://asterix-gerrit.ics.uci.edu/#/c/3386/12/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/HyracksClientInterfaceFunctions.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/HyracksClientInterfaceFunctions.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/12/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/client/HyracksClientInterfaceFunctions.java@410 PS12, Line 410: public boolean extractFromArchive() { isExtract* https://asterix-gerrit.ics.uci.edu/#/c/3386/12/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/12/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@109 PS12, Line 109: String[] fileSplits = urls.get(0).getPath().split("\\."); : String extension = fileSplits[fileSplits.length - 1]; +1 https://asterix-gerrit.ics.uci.edu/#/c/3386/12/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/deployment/DeploymentUtils.java@252 PS12, Line 252: f.delete(); should we try/catch around this so we don't abort delete attempt on first failure? https://asterix-gerrit.ics.uci.edu/#/c/3386/12/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/CCNCFunctions.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/CCNCFunctions.java: https://asterix-gerrit.ics.uci.edu/#/c/3386/12/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/ipc/CCNCFunctions.java@1202 PS12, Line 1202: public boolean extractFromArchive() { isExtract* -- 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: 12 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: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Thu, 27 Jun 2019 15:53:10 +0000 Gerrit-HasComments: Yes
