Upgrade Guava library to version 32.0.0-jre in CDAP Runtime module#16126
Upgrade Guava library to version 32.0.0-jre in CDAP Runtime module#16126AbhishekKumar9984 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request contains a large number of changes primarily focused on replacing deprecated Guava methods (like Throwables.propagate, Closeables.closeQuietly, and Stopwatch.elapsedMillis) with standard Java equivalents or safer alternatives, and updating service lifecycle management from startAndWait/stopAndWait to startAsync().awaitRunning()/stopAsync().awaitTerminated(). While these changes modernize the codebase, several issues were identified regarding performance regressions, potential compatibility issues with newer Java versions, and incorrect exception handling patterns.
| e -> ((ApplicationFilter.ArtifactIdFilter) filter).test( | ||
| e.getValue().getSpec().getArtifactId())); |
There was a problem hiding this comment.
The removal of the getArtifactId() method in AppScanEntry and the change to use e.getValue().getSpec().getArtifactId() here introduces a significant performance regression. e.getValue() triggers a full JSON deserialization of the ApplicationMeta object, which can be quite large and expensive. The previous implementation used a streaming JsonReader to efficiently extract only the required artifactId field without deserializing the entire specification. This optimization should be preserved to maintain performance when scanning applications with artifact filters.
| private static final Gson GSON = new GsonBuilder().registerTypeAdapter(Schema.class, | ||
| new SchemaTypeAdapter()) | ||
| // Starting java 9, some java internal modules are private and cannot be used by current | ||
| // version of gson. https://cdap.atlassian.net/browse/CDAP-21212 | ||
| .setExclusionStrategies(new com.google.gson.ExclusionStrategy() { | ||
| @Override | ||
| public boolean shouldSkipField(com.google.gson.FieldAttributes f) { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean shouldSkipClass(Class<?> clazz) { | ||
| String name = clazz.getName(); | ||
| return name.startsWith("java.lang.module") | ||
| || name.startsWith("jdk.internal") | ||
| || name.startsWith("sun."); | ||
| } | ||
| }) | ||
| .registerTypeAdapter(StructuredRecord.class, new PreviewJsonSerializer()).create(); |
There was a problem hiding this comment.
The removal of the ExclusionStrategy for GSON may cause issues when running on Java 9 or later. This strategy was specifically implemented to avoid reflection-based access to internal JDK modules (like java.lang.module or jdk.internal), which is restricted in newer Java versions and can lead to InaccessibleObjectException. The original code referenced CDAP-21212 as the reason for this inclusion. Please verify if this exclusion is still necessary for compatibility.
| String line; | ||
| try (InputStreamReader reader = new InputStreamReader(Locations.newInputSupplier(resultLocation).getInput(), | ||
| Charsets.UTF_8)) { | ||
| line = CharStreams.readLines(reader).stream().findFirst().orElse(null); |
There was a problem hiding this comment.
CharStreams.readLines(reader) reads the entire content of the file into a list in memory. Since only the first line is needed for verification, this is inefficient if the output file is large. It is better to use a BufferedReader and read only the first line.
| line = CharStreams.readLines(reader).stream().findFirst().orElse(null); | |
| line = new BufferedReader(reader).readLine(); |
| } | ||
|
|
||
| @Test | ||
| @Test |
There was a problem hiding this comment.
The removal of testScanApplicationsWithArtifactFilter and related tests significantly reduces the test coverage for application scanning logic. These tests should be retained to ensure that filtering by artifact still works correctly after the implementation changes, especially given the performance-related changes in the store implementation.
| LOG.error(t.getMessage(), t); | ||
| throw Throwables.propagate(t); | ||
| throw new RuntimeException(t); |
There was a problem hiding this comment.
Replacing Throwables.propagate(t) with new RuntimeException(t) when catching Throwable is problematic. Guava's propagate method would throw Error and RuntimeException types directly, whereas the new code wraps them in a RuntimeException. This changes the exception propagation behavior and can lead to unnecessary exception nesting or incorrect handling of fatal errors. The recommended replacement is to use Throwables.throwIfUnchecked(t) before wrapping.
LOG.error(t.getMessage(), t);
Throwables.throwIfUnchecked(t);
throw new RuntimeException(t);1 participant
Updated the Guava dependency version in pom.xml to 32.0.0-jre.
Addressed compatibility issues with the upgraded Guava version.