From c32297acaaeeac9b806d02e950e31d53acc239fc Mon Sep 17 00:00:00 2001 From: James Turton Date: Tue, 17 Sep 2024 07:29:31 +0200 Subject: [PATCH] Remove Guava patching. --- .../drill/common/util/GuavaPatcher.java | 146 ----------- .../drill/common/util/TestGuavaPatcher.java | 243 ------------------ .../java/org/apache/drill/test/BaseTest.java | 6 - .../appMaster/DrillApplicationMaster.java | 6 - .../apache/drill/yarn/client/DrillOnYarn.java | 8 +- .../apache/drill/exec/server/Drillbit.java | 6 - .../java/org/apache/drill/jdbc/Driver.java | 3 +- 7 files changed, 2 insertions(+), 416 deletions(-) delete mode 100644 common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java delete mode 100644 common/src/test/java/org/apache/drill/common/util/TestGuavaPatcher.java 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 deleted file mode 100644 index ec6c1cabfc8..00000000000 --- a/common/src/main/java/org/apache/drill/common/util/GuavaPatcher.java +++ /dev/null @@ -1,146 +0,0 @@ -/* - * 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 java.lang.reflect.Modifier; - -import com.google.common.annotations.VisibleForTesting; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import javassist.ClassPool; -import javassist.CtClass; -import javassist.CtConstructor; -import javassist.CtMethod; -import javassist.CtNewMethod; -import javassist.scopedpool.ScopedClassPoolRepository; -import javassist.scopedpool.ScopedClassPoolRepositoryImpl; - -public class GuavaPatcher { - private static final Logger logger = LoggerFactory.getLogger(GuavaPatcher.class); - - private static boolean patchingAttempted; - - public static synchronized void patch() { - if (!patchingAttempted) { - patchingAttempted = true; - patchStopwatch(); - patchCloseables(); - patchFuturesCallback(); - } - } - - 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). - */ - private static void patchStopwatch() { - try { - ClassPool cp = getClassPool(); - CtClass cc = cp.get("com.google.common.base.Stopwatch"); - - // Expose the constructor for Stopwatch for old libraries who use the pattern new Stopwatch().start(). - for (CtConstructor c : cc.getConstructors()) { - if (!Modifier.isStatic(c.getModifiers())) { - c.setModifiers(Modifier.PUBLIC); - } - } - - // Add back the Stopwatch.elapsedMillis() method for old consumers. - CtMethod newMethod = CtNewMethod.make( - "public long elapsedMillis() { return elapsed(java.util.concurrent.TimeUnit.MILLISECONDS); }", cc); - cc.addMethod(newMethod); - - // Load the modified class instead of the original. - cc.toClass(); - - logger.info("Google's Stopwatch patched for old HBase Guava version."); - } catch (Exception e) { - logUnableToPatchException(e); - } - } - - private static void logUnableToPatchException(Exception e) { - patchingSuccessful = false; - logger.warn("Unable to patch Guava classes: {}", e.getMessage()); - logger.debug("Exception:", e); - } - - private static void patchCloseables() { - try { - ClassPool cp = getClassPool(); - CtClass cc = cp.get("com.google.common.io.Closeables"); - - // Add back the Closeables.closeQuietly() method for old consumers. - CtMethod newMethod = CtNewMethod.make( - "public static void closeQuietly(java.io.Closeable closeable) { try{closeable.close();}catch(Exception e){} }", - cc); - cc.addMethod(newMethod); - - // Load the modified class instead of the original. - cc.toClass(); - - logger.info("Google's Closeables patched for old HBase Guava version."); - } catch (Exception e) { - logUnableToPatchException(e); - } - } - - /** - * Returns {@link javassist.scopedpool.ScopedClassPool} instance which uses the same class loader - * which was used for loading current class. - * - * @return {@link javassist.scopedpool.ScopedClassPool} instance - */ - private static ClassPool getClassPool() { - ScopedClassPoolRepository classPoolRepository = ScopedClassPoolRepositoryImpl.getInstance(); - // sets prune flag to false to avoid freezing and pruning classes right after obtaining CtClass instance - classPoolRepository.setPrune(false); - return classPoolRepository.createScopedClassPool(GuavaPatcher.class.getClassLoader(), null); - } - - /** - * Makes Guava Futures#addCallback look like the old version for compatibility with hadoop-hdfs (for test purposes). - */ - private static void patchFuturesCallback() { - try { - ClassPool cp = getClassPool(); - CtClass cc = cp.get("com.google.common.util.concurrent.Futures"); - - // Add back the Futures.addCallback(ListenableFuture future, FutureCallback callback) method for old consumers. - CtMethod newMethod = CtNewMethod.make( - "public static void addCallback(" + - "com.google.common.util.concurrent.ListenableFuture future, " + - "com.google.common.util.concurrent.FutureCallback callback" + - ") { addCallback(future, callback, com.google.common.util.concurrent.MoreExecutors.directExecutor()); }", cc); - cc.addMethod(newMethod); - - // Load the modified class instead of the original. - cc.toClass(); - logger.info("Google's Futures#addCallback patched for old Hadoop Guava version."); - } catch (Exception e) { - logUnableToPatchException(e); - } - } -} 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 deleted file mode 100644 index 5dbdf537e7a..00000000000 --- a/common/src/test/java/org/apache/drill/common/util/TestGuavaPatcher.java +++ /dev/null @@ -1,243 +0,0 @@ -/* - * 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 void checkPreconditionsCheckVarargMethod(String methodName, - Class argClass, T goodValue, T badValue, - Class 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 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/common/src/test/java/org/apache/drill/test/BaseTest.java b/common/src/test/java/org/apache/drill/test/BaseTest.java index 6256b5e74c9..92f0d1c26f1 100644 --- a/common/src/test/java/org/apache/drill/test/BaseTest.java +++ b/common/src/test/java/org/apache/drill/test/BaseTest.java @@ -17,7 +17,6 @@ */ package org.apache.drill.test; -import org.apache.drill.common.util.GuavaPatcher; import org.apache.drill.common.util.ProtobufPatcher; /** @@ -33,10 +32,5 @@ public class BaseTest { * This code removes these final modifiers. */ ProtobufPatcher.patch(); - /* - * Some libraries, such as Hadoop, HBase, Iceberg depend on incompatible versions of Guava. - * This code adds back some methods to so that the libraries can work with single Guava version. - */ - GuavaPatcher.patch(); } } diff --git a/drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/DrillApplicationMaster.java b/drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/DrillApplicationMaster.java index e144bff48c1..a74281ba1ff 100644 --- a/drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/DrillApplicationMaster.java +++ b/drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/DrillApplicationMaster.java @@ -19,7 +19,6 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.drill.common.util.GuavaPatcher; import org.apache.drill.common.util.ProtobufPatcher; import org.apache.drill.yarn.appMaster.ControllerFactory.ControllerFactoryException; import org.apache.drill.yarn.appMaster.http.WebServer; @@ -54,11 +53,6 @@ public class DrillApplicationMaster { * This code removes these final modifiers. */ ProtobufPatcher.patch(); - /* - * Some libraries, such as Hadoop or HBase, depend on incompatible versions of Guava. - * This code adds back some methods to so that the libraries can work with single Guava version. - */ - GuavaPatcher.patch(); } private static final Log LOG = LogFactory diff --git a/drill-yarn/src/main/java/org/apache/drill/yarn/client/DrillOnYarn.java b/drill-yarn/src/main/java/org/apache/drill/yarn/client/DrillOnYarn.java index 5f79f5da7f1..650a698eb15 100644 --- a/drill-yarn/src/main/java/org/apache/drill/yarn/client/DrillOnYarn.java +++ b/drill-yarn/src/main/java/org/apache/drill/yarn/client/DrillOnYarn.java @@ -18,7 +18,6 @@ package org.apache.drill.yarn.client; -import org.apache.drill.common.util.GuavaPatcher; import org.apache.drill.common.util.ProtobufPatcher; import org.apache.drill.yarn.core.DoyConfigException; import org.apache.drill.yarn.core.DrillOnYarnConfig; @@ -83,11 +82,6 @@ public class DrillOnYarn { * This code removes these final modifiers. */ ProtobufPatcher.patch(); - /* - * Some libraries, such as Hadoop or HBase, depend on incompatible versions of Guava. - * This code adds back some methods to so that the libraries can work with single Guava version. - */ - GuavaPatcher.patch(); } public static void main(String argv[]) { @@ -190,4 +184,4 @@ private static void displayError(CommandLineOptions opts, ClientException e) { e.printStackTrace(ClientContext.err); } } -} \ No newline at end of file +} diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java index cb78340f7e4..b3ac6eaa1e6 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java @@ -47,7 +47,6 @@ import org.apache.drill.exec.store.sys.store.provider.CachingPersistentStoreProvider; import org.apache.drill.exec.store.sys.store.provider.InMemoryStoreProvider; import org.apache.drill.exec.store.sys.store.provider.LocalPersistentStoreProvider; -import org.apache.drill.common.util.GuavaPatcher; import org.apache.drill.common.util.ProtobufPatcher; import org.apache.drill.exec.work.WorkManager; import com.google.common.annotations.VisibleForTesting; @@ -85,11 +84,6 @@ public class Drillbit implements AutoCloseable { * This code removes these final modifiers. */ ProtobufPatcher.patch(); - /* - * Some libraries, such as Hadoop or HBase, depend on incompatible versions of Guava. - * This code adds back some methods to so that the libraries can work with single Guava version. - */ - GuavaPatcher.patch(); Environment.logEnv("Drillbit environment: ", logger); // Jersey uses java.util.logging - create bridge: jul to slf4j SLF4JBridgeHandler.removeHandlersForRootLogger(); diff --git a/exec/jdbc/src/main/java/org/apache/drill/jdbc/Driver.java b/exec/jdbc/src/main/java/org/apache/drill/jdbc/Driver.java index 6b8dd32970d..74daa3d14c8 100644 --- a/exec/jdbc/src/main/java/org/apache/drill/jdbc/Driver.java +++ b/exec/jdbc/src/main/java/org/apache/drill/jdbc/Driver.java @@ -44,12 +44,11 @@ public class Driver implements java.sql.Driver { static { ProtobufPatcher.patch(); - GuavaPatcher.patch(); // Upon loading of class, register an instance with DriverManager. try { DriverManager.registerDriver(new Driver()); } catch (Error | SQLException e) { - logger.warn("Error in registering Drill JDBC driver {}: {}", Driver.class, e, e); + logger.error("Error in registering Drill JDBC driver {}: {}", Driver.class, e, e); } }