>From Ian Maxon <[email protected]>: Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565 )
Change subject: [NO ISSUE] UDF API improvements ...................................................................... Patch Set 41: (7 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java@184 PS40, Line 184: libraryEntry.put(getDataverseParameter(), dvAndLibs.getKey()); > dvAndLibs.getKey() is a canonical dataverse name. We should not be returning > it as a string. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java@244 PS40, Line 244: public List<Pair<DataverseName, String>> getLibraryListing() throws IOException { > minor. […] i would like to defer if that is ok, because the reason it is factored out like that is because eventually i would like to be able to replace only the libraries which are not in a consistent state on rebalance instead of just wiping it all out. so that method would be called from the bootstrap task in the NC. i had this partially implemented at one point so it is vestigial from that. additionally this method kind of scares me due to the fact it is trying to read something on the filesystem from a HTTP request. so i looked at it really closely as it stands now for any footguns, and I had Mike look at it specifically to be sure it agrees with the on-disk format. so i just have some hesitation about refactoring it right now. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@45 PS40, Line 45: public static Map<String, Map<String, String>> produceLibraryListing(ILibraryManager libraryManager) > See my other comments regarding eliminating this method. addressed in the other comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@48 PS40, Line 48: Map<String, Map<String, String>> dvToLibHashes = new HashMap<>(); > Consider using TreeMap instead of HashMap, so we'd get them sorted by the > dataverse name. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@50 PS40, Line 50: dvToLibHashes.computeIfAbsent(lib.first.getCanonicalForm(), h -> new HashMap<>()).put(lib.getSecond(), > Consider using TreeMap instead of HashMap, so we'd get them sorted by the > library name. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@51 PS40, Line 51: libraryManager.getLibraryHash(lib.first, lib.second)); > getLibraryHash() can theoretically return 'null', but we're not handling it > here. made it throw an exception instead of null. there's nothing to be done if the library is corrupted like that. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java@97 PS40, Line 97: FIND_PYTHON(BOOLEAN, false), > Let's consider changing "find.python" to something that starts with "python. > […] changed it to python.cmd.autolocate -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: cheshire-cat Gerrit-Change-Id: Ib6e6ce7debc9c2e07d24163542c1f98886792165 Gerrit-Change-Number: 9565 Gerrit-PatchSet: 41 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: Tue, 16 Mar 2021 23:33:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Lychagin <[email protected]> Gerrit-MessageType: comment
