>From Ian Maxon <[email protected]>: Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16364 )
Change subject: [ASTERIXDB-3034][RT] Fenced UDFs ...................................................................... Patch Set 11: (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. you are right about 141 and i don't know why that doesn't seem to happen... i put a guard there now anyway. the reason this is here is because the node names are not set in the Podman tests, they exist inside the container 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? in the podman tests it is the case 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. […] unfortuately not really because the exact exception text returned is different depending on whether or not you are using domain sockets. i wanted to not duplicate all the tests so that the two execution methods do not end up regressing in one way or another 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. […] nope. moved into utils 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? done, passed as a param 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. bad refactoring i suppose, these are generic to any external language 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? its a little weird but the reason i throw an IOException is because then the caller of this will catch it and wrap it with a proper AsterixException with the right errorcode 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? good point, guarded 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. to serialize what we discussed: basically if the domain socket path is specified, but is not working for some reason, we should fail here. this is because otherwise it would otherwise be an unsafe fallback to a less secure mode when the user has specified they want the fencing protection. 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 Done 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. ooops. fixed -- 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: 11 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: Mon, 06 Jun 2022 18:11:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Lychagin <[email protected]> Gerrit-MessageType: comment
