Skip to content

Upgrade Guava library to version 32.0.0-jre in cdap#16149

Open
AbhishekKumar9984 wants to merge 1 commit into
cdapio:cs_guava_upgradefrom
cloudsufi:metrics-cloudsufi-guava-upgrade
Open

Upgrade Guava library to version 32.0.0-jre in cdap#16149
AbhishekKumar9984 wants to merge 1 commit into
cdapio:cs_guava_upgradefrom
cloudsufi:metrics-cloudsufi-guava-upgrade

Conversation

@AbhishekKumar9984
Copy link
Copy Markdown

Updated the Guava dependency version in pom.xml to 32.0.0-jre.
Ensured compatibility with the upgraded Guava version.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +63 to +113
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;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. Performance/Exception Overhead: Every call to start() or stop() will always fail the first getMethod("start") / getMethod("stop") lookup, throwing and catching a NoSuchMethodException before falling back to the catch block.
  2. Access Control Issues: Calling getMethod on delegate.getClass() (which is an anonymous class and thus non-public) and invoking it can throw an IllegalAccessException on 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;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant