>From Ian Maxon <[email protected]>: Ian Maxon 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. […] Done 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 . […] Done 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? Done 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? Done 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. […] Done 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. […] Done 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. […] Done 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"? Done 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? it should. not sure why i had it as a boxed 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. […] i can't find a straight answer on whether a boxed type gets incremented or not. i think it changed between java versions or something. but it's a mistake and confusing so fixed. also changed the name 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 […] Done 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. Done 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. […] if you mean in helo() then yes. basically the mid in helo() means nothing. in init()/call() it means function identifier. 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(). […] it is indeed not. we just close the pipe at the moment to kill things. i can remove it. i think we will eventually want it for interpreter reuse 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? […] 1. before call recvBuffer always has its limit set to 0. see in call() or init(). 2. basically in the case there is an exception on the wire, its serialized content lives in bufferBox.getValue(). so IPCHandle.processIncomingMessages() will do it unless read(buf) fails, so i think there is a hole if that happens. i will also check the exception state in the condition to cover that. 3. i have some trepidation about changing the concurrency primitives right now. i agree a semaphore would work but this has been working since August. we are just now finding these last holes in this method of coordination, i am afraid of introducing different ones by changing too much here. if you feel strongly about it though i am happy to do it 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? Done 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. […] yeah i was being too annoyed at having to repeat some of the calls here. fixed 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? Done 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. done, unfortunately we don't have an error code for this i don't think. i want to throw whatever we would throw when a frame cannot be grown in FrameManager, but this isn't a coded exception atm -- 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: Thu, 18 Mar 2021 04:09:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Lychagin <[email protected]> Gerrit-MessageType: comment
