Upgrade Guava library to version 32.0.0-jre in cdap#16149
Upgrade Guava library to version 32.0.0-jre in cdap#16149AbhishekKumar9984 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a dependency on Guava to cdap-data-fabric and introduces a workaround class TxMetricsCollector to satisfy the Tephra TransactionManager. The review feedback highlights that using reflection to invoke start/stop and startAsync/stopAsync on the delegate service is unnecessary and problematic. It introduces performance overhead from caught exceptions and risks IllegalAccessException on newer Java versions. The reviewer suggests directly calling delegate.startAsync() and delegate.stopAsync() instead.
| public ListenableFuture<Service.State> start() { | ||
| try { | ||
| delegate.getClass().getMethod("start").invoke(delegate); | ||
| } catch (Exception e) { | ||
| try { | ||
| delegate.getClass().getMethod("startAsync").invoke(delegate); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| return Futures.immediateFuture(Service.State.RUNNING); | ||
| } | ||
|
|
||
| public ListenableFuture<Service.State> stop() { | ||
| try { | ||
| delegate.getClass().getMethod("stop").invoke(delegate); | ||
| } catch (Exception e) { | ||
| try { | ||
| delegate.getClass().getMethod("stopAsync").invoke(delegate); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| return Futures.immediateFuture(Service.State.TERMINATED); | ||
| } | ||
|
|
||
| public Service startAsync() { | ||
| try { | ||
| delegate.getClass().getMethod("startAsync").invoke(delegate); | ||
| } catch (Exception e) { | ||
| try { | ||
| delegate.getClass().getMethod("start").invoke(delegate); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| public Service stopAsync() { | ||
| try { | ||
| delegate.getClass().getMethod("stopAsync").invoke(delegate); | ||
| } catch (Exception e) { | ||
| try { | ||
| delegate.getClass().getMethod("stop").invoke(delegate); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Since this class is compiled against Guava 32.0.0-jre and CDAP runs with Guava 32.0.0-jre, the delegate (which is a Service) is guaranteed to have the startAsync() and stopAsync() methods at runtime.
Using reflection here is not only unnecessary but also introduces several issues:
- Performance/Exception Overhead: Every call to
start()orstop()will always fail the firstgetMethod("start")/getMethod("stop")lookup, throwing and catching aNoSuchMethodExceptionbefore falling back to thecatchblock. - Access Control Issues: Calling
getMethodondelegate.getClass()(which is an anonymous class and thus non-public) and invoking it can throw anIllegalAccessExceptionon newer Java versions (Java 9+) due to strong encapsulation.
We can safely simplify these methods by calling delegate.startAsync() and delegate.stopAsync() directly.
public ListenableFuture<Service.State> start() {
delegate.startAsync();
return Futures.immediateFuture(Service.State.RUNNING);
}
public ListenableFuture<Service.State> stop() {
delegate.stopAsync();
return Futures.immediateFuture(Service.State.TERMINATED);
}
public Service startAsync() {
delegate.startAsync();
return this;
}
public Service stopAsync() {
delegate.stopAsync();
return this;
}
Updated the Guava dependency version in pom.xml to 32.0.0-jre.
Ensured compatibility with the upgraded Guava version.