You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/06/13 03:21:57 UTC
[GitHub] [flink] zhuzhurk commented on a diff in pull request #19860: [FLINK-27658][table] FlinkUserCodeClassLoader expose addURL method to allow to register jar dynamically
zhuzhurk commented on code in PR #19860:
URL: https://github.com/apache/flink/pull/19860#discussion_r895283370
##########
flink-core/src/main/java/org/apache/flink/util/ClassLoaderUtil.java:
##########
@@ -1,24 +1,22 @@
/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
Review Comment:
Are these newly added spaces unintended changes?
##########
flink-core/src/test/java/org/apache/flink/util/ClassLoaderUtilsTest.java:
##########
@@ -1,24 +1,22 @@
/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
Review Comment:
Are these newly added spaces unintended changes?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/execution/librarycache/RpcInvocationClassLoaderTest.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.execution.librarycache;
+
+import org.apache.flink.runtime.rpc.messages.RemoteRpcInvocation;
+import org.apache.flink.testutils.ClassLoaderUtils;
+import org.apache.flink.util.SerializedValue;
+import org.apache.flink.util.TestLogger;
+
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import java.net.URLClassLoader;
+
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.isA;
+import static org.hamcrest.Matchers.hasItemInArray;
+import static org.hamcrest.Matchers.hasProperty;
+
+/** Tests for rpc invocation utilities. */
+public class RpcInvocationClassLoaderTest extends TestLogger {
Review Comment:
Maybe `ClassLoaderDeserializationTest`? The `RemoteRpcInvocation` is just a tool to do the ser/de-ser.
##########
flink-core/src/main/java/org/apache/flink/util/FlinkUserCodeClassLoader.java:
##########
@@ -67,4 +67,9 @@ protected Class<?> loadClassWithoutExceptionHandling(String name, boolean resolv
throws ClassNotFoundException {
return super.loadClass(name, resolve);
}
+
+ @Override
+ public void addURL(URL url) {
Review Comment:
Looks to me this is not needed.
##########
flink-core/src/test/java/org/apache/flink/util/FlinkUserCodeClassLoadersTest.java:
##########
@@ -1,44 +1,37 @@
/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
Review Comment:
Are these newly added spaces unintended changes?
##########
flink-core/src/main/java/org/apache/flink/util/FlinkUserCodeClassLoaders.java:
##########
@@ -130,7 +129,8 @@ public static class ParentFirstClassLoader extends FlinkUserCodeClassLoader {
* delegate is nulled and can be garbage collected. Additional class resolution will be resolved
* solely through the bootstrap classloader and most likely result in ClassNotFound exceptions.
*/
- private static class SafetyNetWrapperClassLoader extends URLClassLoader implements Closeable {
+ @Internal
Review Comment:
We need to also mark `FlinkUserCodeClassLoaders` as `@Internal`.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/execution/librarycache/RpcInvocationClassLoaderTest.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
Review Comment:
The license format seems not to be aligned with existing files?
##########
flink-core/src/main/java/org/apache/flink/util/ClassLoaderUtil.java:
##########
@@ -1,24 +1,22 @@
/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
*/
-package org.apache.flink.runtime.util;
-
-import org.apache.flink.util.ExceptionUtils;
+package org.apache.flink.util;
Review Comment:
We need to mark this class as `@Internal`.
##########
flink-core/src/test/java/org/apache/flink/util/FlinkUserCodeClassLoadersTest.java:
##########
@@ -53,52 +46,23 @@ public class FlinkUserCodeClassLoadersTest extends TestLogger {
@Rule public ExpectedException expectedException = ExpectedException.none();
- @Test
- public void testMessageDecodingWithUnavailableClass() throws Exception {
- final ClassLoader systemClassLoader = getClass().getClassLoader();
-
- final String className = "UserClass";
- final URLClassLoader userClassLoader =
- ClassLoaderUtils.compileAndLoadJava(
- temporaryFolder.newFolder(),
- className + ".java",
- "import java.io.Serializable;\n"
- + "public class "
- + className
- + " implements Serializable {}");
-
- RemoteRpcInvocation method =
- new RemoteRpcInvocation(
- className,
- "test",
- new Class<?>[] {
- int.class, Class.forName(className, false, userClassLoader)
- },
- new Object[] {
- 1, Class.forName(className, false, userClassLoader).newInstance()
- });
-
- SerializedValue<RemoteRpcInvocation> serializedMethod = new SerializedValue<>(method);
-
- expectedException.expect(ClassNotFoundException.class);
- expectedException.expect(
- allOf(
- isA(ClassNotFoundException.class),
- hasProperty(
- "suppressed",
- hasItemInArray(
- allOf(
- isA(ClassNotFoundException.class),
- hasProperty(
- "message",
- containsString(
- "Could not deserialize 1th parameter type of method test(int, ...).")))))));
-
- RemoteRpcInvocation deserializedMethod =
- serializedMethod.deserializeValue(systemClassLoader);
- deserializedMethod.getMethodName();
-
- userClassLoader.close();
+ public static final String USER_CLASS = "UserClass";
Review Comment:
Can we avoid moving the `TestUserClassLoaderJar` to `flink-core`? Maybe use another class for the testing, like how you do it previously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org