>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364 )
Change subject: [ASTERIXDB-3034][RT] Fenced UDFs ...................................................................... Patch Set 8: (11 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/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/asterixdb/+/16364/8/asterixdb/asterix-app/src/test/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java@135 PS8, Line 135: if (deleteOldInstanceData && nodeNames != null) { how can nodeNames be null? and if it's null then the for loop in line 141 will fail anyway. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/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/asterixdb/+/16364/8/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/LangExecutionUtil.java@160 PS8, Line 160: if(nc != null ){ how can nc be null here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_python.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_python.xml: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_python.xml@71 PS8, Line 71: <expected-warn>ASX0201: External UDF returned exception.</expected-warn> previous warnings were more informative. Is there a way we can get those details back? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILibraryEvaluator.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILibraryEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILibraryEvaluator.java@21 PS8, Line 21: import static org.msgpack.core.MessagePack.Code.ARRAY16; MessagePack use is specific to Pyhton evaluator. Does it need to be in this generic ILibraryEvaluator interface? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILibraryEvaluator.java@43 PS8, Line 43: TaggedValuePointable pointy = TaggedValuePointable.FACTORY.createPointable(); can we avoid object construction here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILibraryEvaluator.java@63 PS8, Line 63: ByteBuffer callPython(long id, IAType[] argTypes, IValueReference[] valueReferences, boolean nullCall) it's not clear why there are Pyhton-specific methods in a generic ILibraryEvaluator interface. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonTCPSocketProto.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonTCPSocketProto.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonTCPSocketProto.java@64 PS8, Line 64: except = new IOException("Python process exited with code: " + proc.exitValue()); should we throw some kind of HyracksException here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryDomainSocketEvaluator.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryDomainSocketEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryDomainSocketEvaluator.java@94 PS8, Line 94: proto.quit(); if can proto, chan, and pid be null here if start() failed? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryEvaluatorFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryEvaluatorFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/PythonLibraryEvaluatorFactory.java@139 PS8, Line 139: if(rt.version().feature() >= 17 && SystemUtils.IS_OS_LINUX){ We should think a bit more about configuration of this feature. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-podman/docker/Dockerfile File asterixdb/asterix-podman/docker/Dockerfile: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/asterix-podman/docker/Dockerfile@11 PS8, Line 11: let's remove this empty line https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/pom.xml File asterixdb/pom.xml: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364/8/asterixdb/pom.xml@373 PS8, Line 373: <!--plugin> we need to uncomment this. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ibeb6228f2dc8edbf642e61cd5633c71913e18972 Gerrit-Change-Number: 16364 Gerrit-PatchSet: 8 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Dmitry Lychagin <[email protected]> Gerrit-Comment-Date: Thu, 02 Jun 2022 00:04:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
