[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=112659&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-112659
]
ASF GitHub Bot logged work on BEAM-3479:
----------------------------------------
Author: ASF GitHub Bot
Created on: 17/Jun/18 21:51
Start Date: 17/Jun/18 21:51
Worklog Time Spent: 10m
Work Description: stale[bot] closed pull request #4412: [BEAM-3479]
adding a test to ensure the right classloader is used to defined the dofninvoker
URL: https://github.com/apache/beam/pull/4412
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java
index 91d9de70c3a..6aaccb500aa 100644
---
a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java
+++
b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java
@@ -20,6 +20,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static org.apache.beam.sdk.util.common.ReflectHelpers.findClassLoader;
+import com.google.common.annotations.VisibleForTesting;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
@@ -95,6 +96,9 @@
public static final String STATE_PARAMETER_METHOD = "state";
public static final String TIMER_PARAMETER_METHOD = "timer";
+ @VisibleForTesting
+ static final String PROXY_CLASSNAME_SUFFIX =
DoFnInvoker.class.getSimpleName();
+
/**
* Returns a {@link ByteBuddyDoFnInvokerFactory} shared with all other
invocations, so that its
* cache of generated classes is global.
@@ -284,7 +288,7 @@ public static RestrictionTracker invokeNewTracker(Object
restriction) {
// private and package-private bits
.with(
StableInvokerNamingStrategy.forDoFnClass(fnClass)
- .withSuffix(DoFnInvoker.class.getSimpleName()))
+ .withSuffix(PROXY_CLASSNAME_SUFFIX))
// class <invoker class> extends DoFnInvokerBase {
.subclass(DoFnInvokerBase.class,
ConstructorStrategy.Default.NO_CONSTRUCTORS)
diff --git
a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java
index 42b9381bc42..7c3550c3003 100644
---
a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java
+++
b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java
@@ -20,6 +20,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
import javax.annotation.Nullable;
import net.bytebuddy.NamingStrategy;
import net.bytebuddy.description.type.TypeDescription;
@@ -31,6 +32,9 @@
*/
@AutoValue
abstract class StableInvokerNamingStrategy extends NamingStrategy.AbstractBase
{
+ /** $ is for a nested class so use as most proxying framework $$. */
+ @VisibleForTesting
+ static final Object PROXY_NAME_DELIMITER = "$$";
public abstract Class<? extends DoFn<?, ?>> getFnClass();
@@ -48,7 +52,9 @@ public StableInvokerNamingStrategy withSuffix(String
newSuffix) {
@Override
protected String name(TypeDescription superClass) {
return String.format(
- "%s$%s",
- getFnClass().getName(), firstNonNull(getSuffix(),
superClass.getName().replace(".", "_")));
+ "%s%s%s",
+ getFnClass().getName(),
+ PROXY_NAME_DELIMITER,
+ firstNonNull(getSuffix(), superClass.getName().replace(".", "_")));
}
}
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactoryTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactoryTest.java
new file mode 100644
index 00000000000..2ce5c005ae8
--- /dev/null
+++
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactoryTest.java
@@ -0,0 +1,74 @@
+/*
+ * 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.beam.sdk.transforms.reflect;
+
+import static java.util.Arrays.asList;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import org.apache.beam.sdk.testing.InterceptingUrlClassLoader;
+import org.apache.beam.sdk.transforms.DoFn;
+import org.junit.Test;
+
+/**
+ * Tests for the proxy generator based on byte buddy.
+ */
+public class ByteBuddyDoFnInvokerFactoryTest {
+ /**
+ * Ensuring we define the subclass using bytebuddy in the right classloader,
+ * i.e. the doFn classloader and not beam classloader.
+ */
+ @Test
+ public void validateProxyClassLoaderSelectionLogic() throws Exception {
+ final ClassLoader testLoader =
Thread.currentThread().getContextClassLoader();
+ final ClassLoader loader = new InterceptingUrlClassLoader(testLoader,
MyDoFn.class.getName());
+ final Class<? extends DoFn<?, ?>> source = (Class<? extends DoFn<?, ?>>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+ assertEquals(loader, source.getClassLoader()); // precondition check
+ final String proxyName = source.getName()
+ + StableInvokerNamingStrategy.PROXY_NAME_DELIMITER
+ + ByteBuddyDoFnInvokerFactory.PROXY_CLASSNAME_SUFFIX;
+ for (final ClassLoader cl : asList(testLoader, loader)) {
+ try {
+ cl.loadClass(proxyName);
+ fail("proxy shouldn't already exist");
+ } catch (final ClassNotFoundException cnfe) {
+ // exactly what we expected!
+ }
+ }
+ final DoFnInvoker<?, ?> subclass = ByteBuddyDoFnInvokerFactory.only()
+ .invokerFor(source.getConstructor().newInstance());
+ try {
+ testLoader.loadClass(proxyName);
+ fail("proxy shouldn't already exist");
+ } catch (final ClassNotFoundException cnfe) {
+ // this is the regression this test prevents
+ }
+ assertEquals(subclass.getClass().getClassLoader(), loader);
+ }
+
+ /**
+ * a sample dofn to test proxy classloader selection.
+ */
+ public static class MyDoFn extends DoFn<String, String> {
+ @ProcessElement
+ public void onElement(final ProcessContext ctx) {
+ // no-op
+ }
+ }
+}
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
index 2c1575ab64c..c68375aeff0 100644
---
a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
+++
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
@@ -779,7 +779,10 @@ public void testStableName() {
invoker.getClass().getName(),
equalTo(
String.format(
- "%s$%s", StableNameTestDoFn.class.getName(),
DoFnInvoker.class.getSimpleName())));
+ "%s%s%s",
+ StableNameTestDoFn.class.getName(),
+ StableInvokerNamingStrategy.PROXY_NAME_DELIMITER,
+ ByteBuddyDoFnInvokerFactory.PROXY_CLASSNAME_SUFFIX)));
}
}
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/OnTimerInvokersTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/OnTimerInvokersTest.java
index af6b6ce2e4f..ea97249b1b4 100644
---
a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/OnTimerInvokersTest.java
+++
b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/OnTimerInvokersTest.java
@@ -131,8 +131,9 @@ public void testStableName() {
invoker.getClass().getName(),
equalTo(
String.format(
- "%s$%s$%s$%s",
+ "%s%s%s$%s$%s",
StableNameTestDoFn.class.getName(),
+ StableInvokerNamingStrategy.PROXY_NAME_DELIMITER,
OnTimerInvoker.class.getSimpleName(),
"timeridwithspecialChars" /* alphanum only; human readable but
not unique */,
"dGltZXItaWQud2l0aCBzcGVjaWFsQ2hhcnN7fQ" /* base64 encoding of
UTF-8 timerId */)));
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 112659)
Time Spent: 3h (was: 2h 50m)
> Add a regression test for the DoFn classloader selection
> --------------------------------------------------------
>
> Key: BEAM-3479
> URL: https://issues.apache.org/jira/browse/BEAM-3479
> Project: Beam
> Issue Type: Task
> Components: sdk-java-core
> Reporter: Romain Manni-Bucau
> Assignee: Romain Manni-Bucau
> Priority: Major
> Fix For: 2.6.0
>
> Time Spent: 3h
> Remaining Estimate: 0h
>
> Follow up task after https://github.com/apache/beam/pull/4235 merge. This
> task is about ensuring we test that.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
