[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=112659=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 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> 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 000..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
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[ https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=112658=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-112658 ] 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] commented on issue #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#issuecomment-397908943 This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 112658) Time Spent: 2h 50m (was: 2h 40m) > 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: 2h 50m > 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[ https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=110493=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-110493 ] ASF GitHub Bot logged work on BEAM-3479: Author: ASF GitHub Bot Created on: 10/Jun/18 20:53 Start Date: 10/Jun/18 20:53 Worklog Time Spent: 10m Work Description: stale[bot] commented on issue #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#issuecomment-396081113 This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the d...@beam.apache.org list. Thank you for your contributions. 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 110493) Time Spent: 2h 40m (was: 2.5h) > 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: 2h 40m > 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=90157=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-90157
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 20:32
Start Date: 11/Apr/18 20:32
Worklog Time Spent: 10m
Work Description: rmannibucau commented on a change in 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#discussion_r180888659
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
That the test ran well and validates what we want even if our test loader
impl changes and is silently broken. They are preconditions checks.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 90157)
Time Spent: 2.5h (was: 2h 20m)
> 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.5.0
>
> Time Spent: 2.5h
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=90144=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-90144
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 19:36
Start Date: 11/Apr/18 19:36
Worklog Time Spent: 10m
Work Description: kennknowles commented on a change in 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#discussion_r180873630
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
What I mean is this: if the classloader is equal, then what is the problem
with the other conditions you are checking?
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 90144)
Time Spent: 2h 20m (was: 2h 10m)
> 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.5.0
>
> Time Spent: 2h 20m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=90120=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-90120
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 18:52
Start Date: 11/Apr/18 18:52
Worklog Time Spent: 10m
Work Description: rmannibucau commented on a change in 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#discussion_r180861562
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
yes, this is what is tested, maybe not the way you think? guess it can just
be a phrasing issue at the end.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 90120)
Time Spent: 2h 10m (was: 2h)
> 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.5.0
>
> Time Spent: 2h 10m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=90104=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-90104
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 18:33
Start Date: 11/Apr/18 18:33
Worklog Time Spent: 10m
Work Description: kennknowles commented on a change in 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#discussion_r180855819
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
The documentation on this test indicates that it is making sure the invoker
classloader equals the DoFn classloader.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 90104)
Time Spent: 2h (was: 1h 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.5.0
>
> Time Spent: 2h
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89937=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89937
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 13:00
Start Date: 11/Apr/18 13:00
Worklog Time Spent: 10m
Work Description: rmannibucau commented on a change in 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#discussion_r180744380
##
File path:
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> source = (Class>)
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(), loader.loadClass(proxyName));
Review comment:
yes loader and it is tested with this line since the class is the one of the
proxy in the loader instance. you can read it as "the invoker classloader is
the fn loader". Will update the line to ensure it is clearer.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 89937)
Time Spent: 1h 50m (was: 1h 40m)
> 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.5.0
>
> Time Spent: 1h 50m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89933=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89933
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 11/Apr/18 12:57
Start Date: 11/Apr/18 12:57
Worklog Time Spent: 10m
Work Description: rmannibucau commented on a change in 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#discussion_r180743571
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
Hmm, I check that block and only the line
assertEquals(loader, source.getClassLoader()); // precondition check
can be removed, the other parts are important cause they validate the setup
is right and we didnt leak an instance due to a previous test making this test
useless.
Is it an issue to keep it?
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 89933)
Time Spent: 1h 40m (was: 1.5h)
> 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.5.0
>
> Time Spent: 1h 40m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89538=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89538
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 10/Apr/18 18:23
Start Date: 10/Apr/18 18:23
Worklog Time Spent: 10m
Work Description: kennknowles commented on a change in 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#discussion_r180521829
##
File path:
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> source = (Class>)
loader.loadClass(
+
"org.apache.beam.sdk.transforms.reflect.ByteBuddyDoFnInvokerFactoryTest$MyDoFn");
+assertEquals(loader, source.getClassLoader()); // precondition check
+final String proxyName = source.getName()
Review comment:
This part is brittle and doesn't really test anything. The stability of the
name is already tested elsewhere. I suggest deleting 43-53.
The assertion in the next block is the whole test.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 89538)
> 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.5.0
>
> Time Spent: 1h 20m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[ https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89537=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89537 ] ASF GitHub Bot logged work on BEAM-3479: Author: ASF GitHub Bot Created on: 10/Apr/18 18:23 Start Date: 10/Apr/18 18:23 Worklog Time Spent: 10m Work Description: kennknowles commented on a change in 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#discussion_r180519642 ## File path: sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java ## @@ -94,6 +94,7 @@ public static final String RESTRICTION_TRACKER_PARAMETER_METHOD = "restrictionTracker"; public static final String STATE_PARAMETER_METHOD = "state"; public static final String TIMER_PARAMETER_METHOD = "timer"; + static final String PROXY_CLASSNAME_SUFFIX = DoFnInvoker.class.getSimpleName(); Review comment: `@VisibleForTesting` 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 89537) > 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.5.0 > > Time Spent: 1h 20m > 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89536=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89536
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 10/Apr/18 18:23
Start Date: 10/Apr/18 18:23
Worklog Time Spent: 10m
Work Description: kennknowles commented on a change in 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#discussion_r180522437
##
File path:
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> source = (Class>)
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(), loader.loadClass(proxyName));
Review comment:
You don't need this. We already separate test that the factory caches the
generated classes. All you should assert in this test is that
`subclass.getLoader()` is `MyDoFn.class.getLoader()`
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 89536)
Time Spent: 1h 20m (was: 1h 10m)
> 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.5.0
>
> Time Spent: 1h 20m
> 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)
[jira] [Work logged] (BEAM-3479) Add a regression test for the DoFn classloader selection
[
https://issues.apache.org/jira/browse/BEAM-3479?focusedWorklogId=89539=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-89539
]
ASF GitHub Bot logged work on BEAM-3479:
Author: ASF GitHub Bot
Created on: 10/Apr/18 18:23
Start Date: 10/Apr/18 18:23
Worklog Time Spent: 10m
Work Description: kennknowles commented on a change in 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#discussion_r180519768
##
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java
##
@@ -31,6 +31,8 @@
*/
@AutoValue
abstract class StableInvokerNamingStrategy extends NamingStrategy.AbstractBase
{
+ /** $ is for a nested class so use as most proxying framework $$. */
+ static final Object PROXY_NAME_DELIMITER = "$$";
Review comment:
`@VisibleForTesting`
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 89539)
Time Spent: 1.5h (was: 1h 20m)
> 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.5.0
>
> Time Spent: 1.5h
> 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)
