[45/50] [abbrv] beam git commit: Use stable naming strategy for ByteBuddy invokers

[ke...@apache.org]

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/jstorm-runner
Commit: 6c45ebfb2832caae99f4992920adbb7d19dce005
Parents: d6cc850
Author: Kenneth Knowles <kl...@google.com>
Authored: Wed Mar 8 17:02:05 2017 -0800
Committer: Kenneth Knowles <kl...@google.com>
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 */)));
+  }
 }