Cs guava upgrade - #16190
Conversation
- Updated the Guava dependency version in `pom.xml` to 32.0.0-jre. - Ensured compatibility with the upgraded Guava version. fixes commnet fixes fixes comments
fixes comments resolved fixes new
fixes haddop fixes fixes
Optimize AppScanEntry artifact filtering to avoid full JSON deserialization Restore GSON ExclusionStrategy in DefaultDataTracer to avoid reflection access errors on Java 9+ Restore testScanApplicationsWithArtifactFilter and testScanApplicationsWithArtifactFilterCoverage in AppMetadataStoreTest.java Clean up try-catch block formatting in TwillAppLifecycleEventHandler.java Clean up formatting and add comments inside empty catch block in AbstractProgramRuntimeService.java Clean up try-catch formatting and add catch block comments in TransactionHttpHandler.java Clean up formatting and add comments inside catch blocks in ArtifactClassLoaderFactory.java Use simple imports for java.io stream types in MapReduceRuntimeService.java Clean up try-catch block formatting in DefaultRuntimeJob.java Clean up try-catch formatting in PluginInstantiator.java Clean up try-catch formatting in DefaultProgramWorkflowRunner.java Use simple imports for java.io streams and java.nio Files in LocalizationUtils.java Clean up try-catch block formatting in HubPackage.java Clean up InterruptedException catch block formatting in OperationalStatsService.java Use Guava Files.copy instead of java.nio.file.Files.copy in HttpHandlerGeneratorTest.java Clean up try-catch block formatting in OperationLifecycleManagerTest.java
Simplify TxMetricsCollector start/stop methods using direct calls to delegate instead of reflection
fixes
…upgrade Upgrade Guava library to version 32.0.0-jre in CDAP common module
Upgrade Guava library to version 32.0.0-jre-router pod
Upgrade Guava library to version 32.0.0-jre-preview pod
…grade Upgrade Guava library to version 32.0.0-jre in CDAP Watchdog module
Upgrade Guava library to version 32.0.0-jre in cdap
…a-upgrade Upgrade Guava library to version 32.0.0-jre-userInterface pod
…va-upgrade Upgrade Guava library to version 32.0.0-jre in CDAP Runtime module
There was a problem hiding this comment.
Code Review
This pull request performs a major cleanup of deprecated Guava and Twill APIs across the codebase, notably replacing synchronous service lifecycle methods (such as startAndWait and stopAndWait) with their asynchronous counterparts (startAsync and stopAsync), replacing Throwables.propagate with standard exceptions, and migrating to MoreObjects and modern CharMatcher and Stopwatch APIs. Additionally, the custom GuavaClassRewriter is removed, and stub suppliers are introduced to maintain compatibility. The review feedback highlights critical issues and improvement opportunities: first, in Services.java, retrieving methods via reflection using service.getClass() instead of Service.class will fail with an IllegalAccessException if the concrete service class is non-public; second, in AppMetadataStore.java, removing IllegalStateException from the catch block during JSON parsing exposes the scan operation to unhandled runtime exceptions from malformed metadata; and third, several files, including LocalizationUtils.java and MapReduceRuntimeService.java, can be simplified by using standard Java NIO Files.copy or native Location stream methods directly instead of deprecated custom supplier wrappers.
| service.getClass().getMethod("startAsync").invoke(service); | ||
| service.getClass().getMethod("awaitRunning").invoke(service); | ||
| } catch (NoSuchMethodException e) { | ||
| // Guava 13 | ||
| Object future = service.getClass().getMethod("start").invoke(service); |
There was a problem hiding this comment.
Using service.getClass().getMethod(...) to retrieve methods via reflection will throw an IllegalAccessException if the concrete class of the Service is non-public (such as package-private or anonymous classes), even if the methods themselves are public. Since Service is a public interface, you should retrieve the methods from Service.class instead of service.getClass().
| service.getClass().getMethod("startAsync").invoke(service); | |
| service.getClass().getMethod("awaitRunning").invoke(service); | |
| } catch (NoSuchMethodException e) { | |
| // Guava 13 | |
| Object future = service.getClass().getMethod("start").invoke(service); | |
| Service.class.getMethod("startAsync").invoke(service); | |
| Service.class.getMethod("awaitRunning").invoke(service); | |
| } catch (NoSuchMethodException e) { | |
| // Guava 13 | |
| Object future = Service.class.getMethod("start").invoke(service); |
| service.getClass().getMethod("stopAsync").invoke(service); | ||
| service.getClass().getMethod("awaitTerminated").invoke(service); | ||
| } catch (NoSuchMethodException e) { | ||
| // Guava 13 | ||
| Object future = service.getClass().getMethod("stop").invoke(service); |
There was a problem hiding this comment.
Using service.getClass().getMethod(...) to retrieve methods via reflection will throw an IllegalAccessException if the concrete class of the Service is non-public (such as package-private or anonymous classes), even if the methods themselves are public. Since Service is a public interface, you should retrieve the methods from Service.class instead of service.getClass().
| service.getClass().getMethod("stopAsync").invoke(service); | |
| service.getClass().getMethod("awaitTerminated").invoke(service); | |
| } catch (NoSuchMethodException e) { | |
| // Guava 13 | |
| Object future = service.getClass().getMethod("stop").invoke(service); | |
| Service.class.getMethod("stopAsync").invoke(service); | |
| Service.class.getMethod("awaitTerminated").invoke(service); | |
| } catch (NoSuchMethodException e) { | |
| // Guava 13 | |
| Object future = Service.class.getMethod("stop").invoke(service); |
| } catch (IOException e) { | ||
| LOG.warn("Failed to extract artifact id from raw application metadata", e); | ||
| } |
There was a problem hiding this comment.
The JsonReader operations (such as beginObject, peek, nextName, and skipValue) as well as GSON.fromJson can throw IllegalStateException or JsonSyntaxException (which are RuntimeExceptions) if the JSON metadata is malformed or has an unexpected structure (e.g., if spec is a string instead of an object). Since you removed IllegalStateException from the catch block, any such malformed metadata in the database will cause the entire scan operation to fail with an unhandled exception. Please catch Exception or IOException | RuntimeException to safely handle and log these parsing errors.
| } catch (IOException e) { | |
| LOG.warn("Failed to extract artifact id from raw application metadata", e); | |
| } | |
| } catch (Exception e) { | |
| LOG.warn("Failed to extract artifact id from raw application metadata", e); | |
| } |
| try (InputStream in = Resources.asByteSource(url).openStream(); | ||
| OutputStream out = new FileOutputStream(tempFile)) { | ||
| com.google.common.io.ByteStreams.copy(in, out); | ||
| } |
There was a problem hiding this comment.
Instead of manually opening a FileOutputStream and using ByteStreams.copy, you can use the standard Java NIO Files.copy method to copy the InputStream directly to the target path. This is cleaner, more concise, and utilizes optimized platform-specific copy operations.
try (InputStream in = Resources.asByteSource(url).openStream()) {
Files.copy(in, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
}| try (InputStream in = new FileInputStream(file); | ||
| OutputStream out = Locations.newOutputSupplier(targetLocation).getOutput()) { | ||
| ByteStreams.copy(in, out); | ||
| } |
There was a problem hiding this comment.
Since Location provides a direct getOutputStream() method, you can use it directly instead of wrapping it with Locations.newOutputSupplier. This simplifies the code and avoids using the custom/deprecated supplier wrapper.
| try (InputStream in = new FileInputStream(file); | |
| OutputStream out = Locations.newOutputSupplier(targetLocation).getOutput()) { | |
| ByteStreams.copy(in, out); | |
| } | |
| try (InputStream in = new FileInputStream(file); | |
| OutputStream out = targetLocation.getOutputStream()) { | |
| ByteStreams.copy(in, out); | |
| } |
| try (InputStream in = Locations.newInputSupplier(programJarLocation).getInput(); | ||
| OutputStream out = Locations.newOutputSupplier(programJarCopy).getOutput()) { | ||
| ByteStreams.copy(in, out); | ||
| } |
There was a problem hiding this comment.
Since both programJarLocation and programJarCopy are Location objects, you can use their native getInputStream() and getOutputStream() methods directly instead of wrapping them with Locations.newInputSupplier and Locations.newOutputSupplier. This simplifies the code and avoids using the custom/deprecated supplier wrappers.
| try (InputStream in = Locations.newInputSupplier(programJarLocation).getInput(); | |
| OutputStream out = Locations.newOutputSupplier(programJarCopy).getOutput()) { | |
| ByteStreams.copy(in, out); | |
| } | |
| try (InputStream in = programJarLocation.getInputStream(); | |
| OutputStream out = programJarCopy.getOutputStream()) { | |
| ByteStreams.copy(in, out); | |
| } |
No description provided.