Repository: beam Updated Branches: refs/heads/master 467e68f93 -> 5e3c5c657
Use stable naming strategy for ByteBuddy invokers This helps to coalesce related failures across multiple JVM instances, over time, across machines, etc. Project: http://git-wip-us.apache.org/repos/asf/beam/repo Commit: http://git-wip-us.apache.org/repos/asf/beam/commit/6c45ebfb Tree: http://git-wip-us.apache.org/repos/asf/beam/tree/6c45ebfb Diff: http://git-wip-us.apache.org/repos/asf/beam/diff/6c45ebfb Branch: refs/heads/master Commit: 6c45ebfb2832caae99f4992920adbb7d19dce005 Parents: d6cc850 Author: Kenneth Knowles <[email protected]> Authored: Wed Mar 8 17:02:05 2017 -0800 Committer: Kenneth Knowles <[email protected]> Committed: Wed May 17 10:40:00 2017 -0700 ---------------------------------------------------------------------- .../reflect/ByteBuddyDoFnInvokerFactory.java | 10 ++-- .../reflect/ByteBuddyOnTimerInvokerFactory.java | 20 ++++---- .../reflect/StableInvokerNamingStrategy.java | 54 ++++++++++++++++++++ .../transforms/reflect/DoFnInvokersTest.java | 19 +++++++ .../transforms/reflect/OnTimerInvokersTest.java | 32 ++++++++++++ 5 files changed, 118 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/beam/blob/6c45ebfb/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java ---------------------------------------------------------------------- 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 8ae2c65..5d5887a 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 @@ -29,7 +29,6 @@ import java.util.List; import java.util.Map; import javax.annotation.Nullable; import net.bytebuddy.ByteBuddy; -import net.bytebuddy.NamingStrategy; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.modifier.Visibility; @@ -282,12 +281,9 @@ public class ByteBuddyDoFnInvokerFactory implements DoFnInvokerFactory { // Create subclasses inside the target class, to have access to // private and package-private bits .with( - new NamingStrategy.SuffixingRandom("auxiliary") { - @Override - public String subclass(TypeDescription.Generic superClass) { - return super.name(clazzDescription); - } - }) + StableInvokerNamingStrategy.forDoFnClass(fnClass) + .withSuffix(DoFnInvoker.class.getSimpleName())) + // class <invoker class> extends DoFnInvokerBase { .subclass(DoFnInvokerBase.class, ConstructorStrategy.Default.NO_CONSTRUCTORS) http://git-wip-us.apache.org/repos/asf/beam/blob/6c45ebfb/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyOnTimerInvokerFactory.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyOnTimerInvokerFactory.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyOnTimerInvokerFactory.java index 123808c..e031337 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyOnTimerInvokerFactory.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyOnTimerInvokerFactory.java @@ -21,12 +21,12 @@ import com.google.common.base.CharMatcher; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; +import com.google.common.io.BaseEncoding; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.concurrent.ExecutionException; import net.bytebuddy.ByteBuddy; -import net.bytebuddy.NamingStrategy; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.modifier.FieldManifestation; import net.bytebuddy.description.modifier.Visibility; @@ -150,20 +150,20 @@ class ByteBuddyOnTimerInvokerFactory implements OnTimerInvokerFactory { final TypeDescription clazzDescription = new TypeDescription.ForLoadedType(fnClass); - final String className = - "auxiliary_OnTimer_" + CharMatcher.JAVA_LETTER_OR_DIGIT.retainFrom(timerId); + final String suffix = + String.format( + "%s$%s$%s", + OnTimerInvoker.class.getSimpleName(), + CharMatcher.javaLetterOrDigit().retainFrom(timerId), + BaseEncoding.base64().omitPadding().encode(timerId.getBytes())); DynamicType.Builder<?> builder = new ByteBuddy() // Create subclasses inside the target class, to have access to // private and package-private bits - .with( - new NamingStrategy.SuffixingRandom(className) { - @Override - public String subclass(TypeDescription.Generic superClass) { - return super.name(clazzDescription); - } - }) + .with(StableInvokerNamingStrategy.forDoFnClass(fnClass) + .withSuffix(suffix)) + // class <invoker class> implements OnTimerInvoker { .subclass(OnTimerInvoker.class, ConstructorStrategy.Default.NO_CONSTRUCTORS) http://git-wip-us.apache.org/repos/asf/beam/blob/6c45ebfb/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java ---------------------------------------------------------------------- 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 new file mode 100644 index 0000000..42b9381 --- /dev/null +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/StableInvokerNamingStrategy.java @@ -0,0 +1,54 @@ +/* + * 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 com.google.common.base.MoreObjects.firstNonNull; + +import com.google.auto.value.AutoValue; +import javax.annotation.Nullable; +import net.bytebuddy.NamingStrategy; +import net.bytebuddy.description.type.TypeDescription; +import org.apache.beam.sdk.transforms.DoFn; + +/** + * A naming strategy for ByteBuddy invokers ({@link DoFnInvoker} and {@link OnTimerInvoker}) that is + * deterministic and readable. This is correct to use only when a class is created at most once. + */ +@AutoValue +abstract class StableInvokerNamingStrategy extends NamingStrategy.AbstractBase { + + public abstract Class<? extends DoFn<?, ?>> getFnClass(); + + @Nullable + public abstract String getSuffix(); + + public static StableInvokerNamingStrategy forDoFnClass(Class<? extends DoFn<?, ?>> fnClass) { + return new AutoValue_StableInvokerNamingStrategy(fnClass, null); + } + + public StableInvokerNamingStrategy withSuffix(String newSuffix) { + return new AutoValue_StableInvokerNamingStrategy(getFnClass(), newSuffix); + } + + @Override + protected String name(TypeDescription superClass) { + return String.format( + "%s$%s", + getFnClass().getName(), firstNonNull(getSuffix(), superClass.getName().replace(".", "_"))); + } +} http://git-wip-us.apache.org/repos/asf/beam/blob/6c45ebfb/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java ---------------------------------------------------------------------- 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 a8cd35e..3edb194 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 @@ -694,4 +694,23 @@ public class DoFnInvokersTest { invoker.invokeOnTimer(timerId, mockArgumentProvider); assertThat(fn.window, equalTo(testWindow)); } + + static class StableNameTestDoFn extends DoFn<Void, Void> { + @ProcessElement + public void process() {} + }; + + /** + * This is a change-detector test that the generated name is stable across runs. + */ + @Test + public void testStableName() { + DoFnInvoker<Void, Void> invoker = DoFnInvokers.invokerFor(new StableNameTestDoFn()); + assertThat( + invoker.getClass().getName(), + equalTo( + String.format( + "%s$%s", StableNameTestDoFn.class.getName(), DoFnInvoker.class.getSimpleName()))); + } + } http://git-wip-us.apache.org/repos/asf/beam/blob/6c45ebfb/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/OnTimerInvokersTest.java ---------------------------------------------------------------------- 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 d317952..0cc67c6 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 @@ -105,4 +105,36 @@ public class OnTimerInvokersTest { this.window = window; } } + + static class StableNameTestDoFn extends DoFn<Void, Void> { + private static final String TIMER_ID = "timer-id.with specialChars{}"; + + @TimerId(TIMER_ID) + private final TimerSpec myTimer = TimerSpecs.timer(TimeDomain.PROCESSING_TIME); + + @ProcessElement + public void process() {} + + @OnTimer(TIMER_ID) + public void onMyTimer() {} + }; + + /** + * This is a change-detector test that the generated name is stable across runs. + */ + @Test + public void testStableName() { + OnTimerInvoker<Void, Void> invoker = + OnTimerInvokers.forTimer(new StableNameTestDoFn(), StableNameTestDoFn.TIMER_ID); + + assertThat( + invoker.getClass().getName(), + equalTo( + String.format( + "%s$%s$%s$%s", + StableNameTestDoFn.class.getName(), + OnTimerInvoker.class.getSimpleName(), + "timeridwithspecialChars" /* alphanum only; human readable but not unique */, + "dGltZXItaWQud2l0aCBzcGVjaWFsQ2hhcnN7fQ" /* base64 encoding of UTF-8 timerId */))); + } }
