This is an automated email from the ASF dual-hosted git repository.
laurent pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/drill.git
The following commit(s) were added to refs/heads/master by this push:
new fe85f83 DRILL-7945: Stop patching newly added Guava methods (#2247)
fe85f83 is described below
commit fe85f835ccf5b88d62e383ff34907f3a703223c6
Author: Laurent Goujon <[email protected]>
AuthorDate: Fri Jun 4 13:55:56 2021 -0700
DRILL-7945: Stop patching newly added Guava methods (#2247)
In order to support Apache Iceberg, GuavaPatcher adds several methods to
com.google.common.base.Preconditions required by Apache Iceberg runtime
but not available in Guava 18.
When Guava version was updated to 30.1.1, Guava Patcher tries to update
existing methods with some code referencing package protected methods
which do no exist anymore. Patching fails but as the methods do now
exists, there's no compatibility issue. That said, GuavaPatcher emits
some logging which might confuse end user.
Remove the unnecessary patching of the Preconditions class and also add
some unit test designed to fail if patching fails for any reason.
---
.../org/apache/drill/common/util/GuavaPatcher.java | 118 +---------
.../apache/drill/common/util/TestGuavaPatcher.java | 243 +++++++++++++++++++++
src/main/resources/checkstyle-config.xml | 1 +
3 files changed, 256 insertions(+), 106 deletions(-)
diff --git
a/common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java
b/common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java
index 0a0a7b2..cd13366 100644
--- a/common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java
+++ b/common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java
@@ -18,11 +18,10 @@
package org.apache.drill.common.util;
import java.lang.reflect.Modifier;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.List;
-import java.util.stream.Collectors;
-import java.util.stream.IntStream;
+
+import
org.apache.drill.shaded.guava.com.google.common.annotations.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import javassist.ClassPool;
import javassist.CtClass;
@@ -31,8 +30,6 @@ import javassist.CtMethod;
import javassist.CtNewMethod;
import javassist.scopedpool.ScopedClassPoolRepository;
import javassist.scopedpool.ScopedClassPoolRepositoryImpl;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
public class GuavaPatcher {
private static final Logger logger =
LoggerFactory.getLogger(GuavaPatcher.class);
@@ -44,10 +41,16 @@ public class GuavaPatcher {
patchingAttempted = true;
patchStopwatch();
patchCloseables();
- patchPreconditions();
}
}
+ private static boolean patchingSuccessful = true;
+
+ @VisibleForTesting
+ static boolean isPatchingSuccessful() {
+ return patchingAttempted && patchingSuccessful;
+ }
+
/**
* Makes Guava stopwatch look like the old version for compatibility with
hbase-server (for test purposes).
*/
@@ -78,6 +81,7 @@ public class GuavaPatcher {
}
private static void logUnableToPatchException(Exception e) {
+ patchingSuccessful = false;
logger.warn("Unable to patch Guava classes: {}", e.getMessage());
logger.debug("Exception:", e);
}
@@ -103,104 +107,6 @@ public class GuavaPatcher {
}
/**
- * Patches Guava Preconditions with missing methods, added for the Apache
Iceberg.
- */
- private static void patchPreconditions() {
- try {
- ClassPool cp = getClassPool();
- CtClass cc = cp.get("com.google.common.base.Preconditions");
-
- // Javassist does not support varargs, generate methods with varying
number of arguments
- int startIndex = 1;
- int endIndex = 5;
-
- List<String> methodsWithVarargsTemplates = Arrays.asList(
- "public static void checkArgument(boolean expression, String
errorMessageTemplate, %s) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalArgumentException(format(errorMessageTemplate, new Object[] { %s }));\n"
- + " }\n"
- + " }",
-
- "public static Object checkNotNull(Object reference, String
errorMessageTemplate, %s) {\n"
- + " if (reference == null) {\n"
- + " throw new
NullPointerException(format(errorMessageTemplate, new Object[] { %s }));\n"
- + " } else {\n"
- + " return reference;\n"
- + " }\n"
- + " }",
-
- "public static void checkState(boolean expression, String
errorMessageTemplate, %s) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalStateException(format(errorMessageTemplate, new Object[] { %s }));\n"
- + " }\n"
- + " }"
- );
-
- List<String> methodsWithPrimitives = Arrays.asList(
- "public static void checkArgument(boolean expression, String
errorMessageTemplate, int arg1) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalArgumentException(format(errorMessageTemplate, new Object[] { new
Integer(arg1) }));\n"
- + " }\n"
- + " }",
- "public static void checkArgument(boolean expression, String
errorMessageTemplate, long arg1) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalArgumentException(format(errorMessageTemplate, new Object[] { new
Long(arg1) }));\n"
- + " }\n"
- + " }",
- "public static void checkArgument(boolean expression, String
errorMessageTemplate, long arg1, long arg2) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalArgumentException(format(errorMessageTemplate, new Object[] { new
Long(arg1), new Long(arg2)}));\n"
- + " }\n"
- + " }",
- "public static Object checkNotNull(Object reference, String
errorMessageTemplate, int arg1) {\n"
- + " if (reference == null) {\n"
- + " throw new
NullPointerException(format(errorMessageTemplate, new Object[] { new
Integer(arg1) }));\n"
- + " } else {\n"
- + " return reference;\n"
- + " }\n"
- + " }",
- "public static void checkState(boolean expression, String
errorMessageTemplate, int arg1) {\n"
- + " if (!expression) {\n"
- + " throw new
IllegalStateException(format(errorMessageTemplate, new Object[] { new
Integer(arg1) }));\n"
- + " }\n"
- + " }"
- );
-
- List<String> newMethods = IntStream.rangeClosed(startIndex, endIndex)
- .mapToObj(
- i -> {
- List<String> args = IntStream.rangeClosed(startIndex, i)
- .mapToObj(j -> "arg" + j)
- .collect(Collectors.toList());
-
- String methodInput = args.stream()
- .map(arg -> "Object " + arg)
- .collect(Collectors.joining(", "));
-
- String arrayInput = String.join(", ", args);
-
- return methodsWithVarargsTemplates.stream()
- .map(method -> String.format(method, methodInput,
arrayInput))
- .collect(Collectors.toList());
- })
- .flatMap(Collection::stream)
- .collect(Collectors.toList());
-
- newMethods.addAll(methodsWithPrimitives);
-
- for (String method : newMethods) {
- CtMethod newMethod = CtNewMethod.make(method, cc);
- cc.addMethod(newMethod);
- }
-
- cc.toClass();
- logger.info("Google's Preconditions were patched to hold new methods.");
- } catch (Exception e) {
- logUnableToPatchException(e);
- }
- }
-
- /**
* Returns {@link javassist.scopedpool.ScopedClassPool} instance which uses
the same class loader
* which was used for loading current class.
*
diff --git
a/common/src/test/java/org/apache/drill/common/util/TestGuavaPatcher.java
b/common/src/test/java/org/apache/drill/common/util/TestGuavaPatcher.java
new file mode 100644
index 0000000..5dbdf53
--- /dev/null
+++ b/common/src/test/java/org/apache/drill/common/util/TestGuavaPatcher.java
@@ -0,0 +1,243 @@
+/*
+ * 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.drill.common.util;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.drill.test.BaseTest;
+import org.junit.Test;
+
+// Easier to test Guava patching if we can reference Guava classes directly
for the tests
+// CHECKSTYLE:OFF
+import com.google.common.base.Preconditions;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Ticker;
+import com.google.common.io.Closeables;
+//CHECKSTYLE:ON
+
+/**
+ * Test class for {@code GuavaPatcher}
+ *
+ */
+public class TestGuavaPatcher extends BaseTest {
+
+ @Test
+ public void checkSuccessfulPatching() {
+ // Catch-all test to see if Guava patching was successful
+ assertTrue("Guava Patcher ran with errors, check error log messages",
+ GuavaPatcher.isPatchingSuccessful());
+ }
+
+ @Test
+ public void checkStopwatchEllapsedMillis() throws Exception {
+ long[] currentTimeMillis = new long[] { 0L };
+ final Ticker ticker = new Ticker() {
+ @Override
+ public long read() {
+ return TimeUnit.MILLISECONDS.toNanos(currentTimeMillis[0]);
+ }
+ };
+ final Stopwatch stopwatch = Stopwatch.createStarted(ticker);
+ currentTimeMillis[0] = 12345L;
+ stopwatch.stop();
+
+ assertEquals(currentTimeMillis[0],
+ stopwatch.elapsed(TimeUnit.MILLISECONDS));
+ assertEquals(currentTimeMillis[0], (long) invokeMethod(Stopwatch.class,
+ "elapsedMillis", new Class[] {}, stopwatch));
+ }
+
+ @Test
+ public void checkCloseablesCloseQuietly() throws Exception {
+ final Closeable alwaysThrows = () -> {
+ throw new IOException("Always fail");
+ };
+
+ invokeMethod(Closeables.class, "closeQuietly",
+ new Class[] { Closeable.class }, null, alwaysThrows);
+ }
+
+ // All the preconditions checks are method which were previously added for
+ // compatibility with Apache Iceberg but are not necessary anymore because
+ // Guava's version has been updated since.
+
+ @Test
+ public void checkPreconditionsCheckArgumentIntParam() throws Exception {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, int.class }, null, true,
+ "Error Message %s", 1);
+ try {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, int.class }, null, false,
+ "Error Message %s", 1);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(IllegalArgumentException.class, cause.getClass());
+ assertEquals("Error Message 1", cause.getMessage());
+ }
+ }
+
+ @Test
+ public void checkPreconditionsCheckArgumentLongParam() throws Exception {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, long.class }, null, true,
+ "Error Message %s", 2L);
+ try {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, long.class }, null, false,
+ "Error Message %s", 2L);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(IllegalArgumentException.class, cause.getClass());
+ assertEquals("Error Message 2", cause.getMessage());
+ }
+ }
+
+ @Test
+ public void checkPreconditionsCheckArgumentLongLongParam() throws Exception {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, long.class, long.class },
+ null, true, "Error Message %s %s", 3L, 4L);
+ try {
+ invokeMethod(Preconditions.class, "checkArgument",
+ new Class[] { boolean.class, String.class, long.class, long.class },
+ null, false, "Error Message %s %s", 3L, 4L);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(IllegalArgumentException.class, cause.getClass());
+ assertEquals("Error Message 3 4", cause.getMessage());
+ }
+ }
+
+ @Test
+ public void checkPreconditionsCheckNotNullIntParam() throws Exception {
+ invokeMethod(Preconditions.class, "checkNotNull",
+ new Class[] { Object.class, String.class, int.class }, null, this,
+ "Error Message %s", 5);
+ try {
+ invokeMethod(Preconditions.class, "checkNotNull",
+ new Class[] { Object.class, String.class, int.class }, null, null,
+ "Error Message %s", 5);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(NullPointerException.class, cause.getClass());
+ assertEquals("Error Message 5", cause.getMessage());
+ }
+ }
+
+ @Test
+ public void checkPreconditionsCheckStateIntParam() throws Exception {
+ invokeMethod(Preconditions.class, "checkState",
+ new Class[] { boolean.class, String.class, int.class }, null, true,
+ "Error Message %s", 6);
+ try {
+ invokeMethod(Preconditions.class, "checkState",
+ new Class[] { boolean.class, String.class, int.class }, null, false,
+ "Error Message %s", 6);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(IllegalStateException.class, cause.getClass());
+ assertEquals("Error Message 6", cause.getMessage());
+ }
+ }
+
+ @Test
+ public void checkPreconditionsCheckNotNullVarargs() throws Exception {
+ checkPreconditionsCheckVarargMethod("checkNotNull", Object.class, this,
+ null, NullPointerException.class);
+ }
+
+ @Test
+ public void checkPreconditionsCheckArgumentVarargs() throws Exception {
+ checkPreconditionsCheckVarargMethod("checkArgument", boolean.class, true,
+ false, IllegalArgumentException.class);
+ }
+
+ @Test
+ public void checkPreconditionsCheckStateVarargs() throws Exception {
+ checkPreconditionsCheckVarargMethod("checkState", boolean.class, true,
+ false, IllegalStateException.class);
+ }
+
+ private <T> void checkPreconditionsCheckVarargMethod(String methodName,
+ Class<T> argClass, T goodValue, T badValue,
+ Class<? extends Throwable> exceptionClass) throws Exception {
+ for (int i = 1; i < 5; i++) {
+ String[] templatePlaceholders = new String[i];
+ Arrays.fill(templatePlaceholders, "%s");
+
+ Object[] templateArguments = new Object[i];
+ Arrays.setAll(templateArguments, Integer::valueOf);
+
+ String template = "Error message: "
+ + Stream.of(templatePlaceholders).collect(Collectors.joining(","));
+ String message = "Error message: " + Stream.of(templateArguments)
+ .map(Object::toString).collect(Collectors.joining(","));
+
+ Class<?>[] parameterTypes = new Class[2 + i];
+ parameterTypes[0] = argClass;
+ parameterTypes[1] = String.class;
+ Arrays.fill(parameterTypes, 2, parameterTypes.length, Object.class);
+
+ Object[] parameters = new Object[2 + i];
+ parameters[0] = goodValue;
+ parameters[1] = template;
+ System.arraycopy(templateArguments, 0, parameters, 2, i);
+
+ // Check successful call
+ invokeMethod(Preconditions.class, methodName, parameterTypes, null,
+ parameters);
+
+ try {
+ parameters[0] = badValue;
+ invokeMethod(Preconditions.class, methodName, parameterTypes, null,
+ parameters);
+ fail();
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ assertEquals(exceptionClass, cause.getClass());
+ assertEquals(message, cause.getMessage());
+ }
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ private static <T> T invokeMethod(Class<?> clazz, String methodName,
+ Class<?>[] argumentTypes, Object object, Object... arguments)
+ throws Exception {
+ Method method = clazz.getMethod(methodName, argumentTypes);
+ return (T) method.invoke(object, arguments);
+ }
+
+}
diff --git a/src/main/resources/checkstyle-config.xml
b/src/main/resources/checkstyle-config.xml
index f444cc4..488985d 100644
--- a/src/main/resources/checkstyle-config.xml
+++ b/src/main/resources/checkstyle-config.xml
@@ -41,6 +41,7 @@
<module name="EmptyStatement"/>
<module name="NoWhitespaceBefore"/>
<module name="OneStatementPerLine"/>
+ <module name="SuppressionCommentFilter"/>
</module>
