>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187 )
Change subject: [ASTERIXDB-2838][RT][FUN] Batched PyUDF calls ...................................................................... Patch Set 38: (19 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.10.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.10.query.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.10.query.sqlpp@26 PS37, Line 26: select value count(t.text) We don't put two queries into a single .query file. Can you split this into two files or rewrite as cross-product or union-all? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.9.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.9.query.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/mysentiment_twitter/mysentiment_twitter.9.query.sqlpp@24 PS37, Line 24: select value count(t.text) split into 2 .sqlpp files https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java@37 PS37, Line 37: AtomicLong maxId = new AtomicLong(0); can we make all fields private final? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java@38 PS37, Line 38: ConcurrentHashMap<Long, MutableObject<ByteBuffer>> activeClients = new ConcurrentHashMap<>(); may be we should use just 1 map Map<Long, Pair<ByteBuffer, Exception>> here instead of 2 maps? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java@61 PS37, Line 61: synchronized (activeClients.get(rmid)) { minor. can we assign activeClients.get(rmid) to a variable and use that variable in lines 61 and 63? It's also used in lines 48 and 56 above. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java@84 PS37, Line 84: public Exception getException(Long id) { minor. I think we should rename this method to getAndRemoveException() or something similar, to indicate that it has a side-effect. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-common/src/main/java/org/apache/asterix/external/ipc/ExternalFunctionResultRouter.java@88 PS37, Line 88: public boolean hasException(long id) { Minor. May be remove this method at all? The caller code would just call getException() if that's not null then the returned Exception is the one. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@43 PS37, Line 43: Long initKey; may be rename "initKey" to "routeId"? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@46 PS37, Line 46: Long maxFunctionId; should maxFunctionId be declared as primitive long type? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@80 PS37, Line 80: long key = maxFunctionId++; 1. seem like maxFunctionId++ is not doing the intended thing here because maxFunctionId is declared as Long. 2. can we call this variable "functionId" instead of "key". We already have "initKey" field in this method which means something else. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@95 PS37, Line 95: public long init(String module, String fn) throws IOException, AsterixException { is there a way we could have just one init() method to avoid code duplication here? may be it could take 3 parameters: module, clazz, fn with clazz allowed to be null if the function is not inside the class. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@96 PS37, Line 96: long key = maxFunctionId++; same comment about incrementing Long as in line 80 above. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@103 PS37, Line 103: sendMsg(key); just curious. we sent "routeId" in the first sendMsg() in line 87 above, but then switch to sending "functionId" in sendMsg(). This is intentional, right? Just double checking. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@141 PS37, Line 141: public void quit() { I can't seem to find any callers of quit(). Is it called? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@150 PS37, Line 150: while (bufferBox.getValue().limit() == 0 && pythonProc.isAlive()) { 1. which code resets the limit to 0? 2. in case of an exception (router.hasException()==true), which code sets the limit to non-zero? 3. May be should consider just using Semaphore for signaling, instead of wait/notify on the buffer mutable. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonIPCProto.java@172 PS37, Line 172: try (MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(recvBuffer)) { any chance we can reuse the unpacker here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java@90 PS37, Line 90: for (String s : initAry) { minor. there's no need to allocate a new array here. we can compute dataLength just by adding 3 lengths together. we can also just write the 3 parts as is (so no need to loop over this array) in line 90. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java@95 PS37, Line 95: public void init(final String module, final String fn) { can we combine these two init() methods into one? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187/37/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java@135 PS37, Line 135: //TODO: something more graceful yes. we need to throw HyrackException here. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/10187 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: I5af4da999985afcc33cdfacea79576f1d6109173 Gerrit-Change-Number: 10187 Gerrit-PatchSet: 38 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-CC: Till Westmann <[email protected]> Gerrit-Comment-Date: Wed, 17 Mar 2021 05:57:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
