Logs warning message when Batcher is not closed on GC by rahulKQL · Pull Request #746 · googleapis/gax-java

rahulKQL · 2019-06-27T14:17:13Z

This is a followUp PR to#734

What this PR contains

Displays a warning in the console when Batchers are garbage collected without calling to Batcher#close().

Derived from ManagedChannelImpl.ManagedChannelReference.

rahulKQL · 2019-06-27T15:12:08Z

I think Travis needs to be retried because it's failing with:

* What went wrong:
Could not resolve all files for configuration ':gax:compileClasspath'.
> Could not resolve com.google.code.findbugs:jsr305:3.0.2.
  Required by:
      project :gax
      project :gax > com.google.guava:guava:28.0-android
      project :gax > com.google.auth:google-auth-library-oauth2-http:0.16.1 > com.google.http-client:google-http-client:1.30.1
   > Could not resolve com.google.code.findbugs:jsr305:3.0.2.
      > Could not get resource 'https://repo.maven.apache.org/maven2/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.pom'.
         > Could not GET 'https://repo.maven.apache.org/maven2/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.pom'. Received status code 403 from server: Forbidden

rahulKQL · 2019-06-27T18:49:58Z

@igorbernstein2 Please have a look at this change.

codecov · 2019-06-28T09:40:20Z

Codecov Report

Merging #746 into master will increase coverage by 0.12%.
The diff coverage is 92.5%.

@@             Coverage Diff              @@
##             master     #746      +/-   ##
============================================
+ Coverage     77.72%   77.84%   +0.12%     
- Complexity     1094     1095       +1     
============================================
  Files           198      198              
  Lines          4799     4839      +40     
  Branches        377      381       +4     
============================================
+ Hits           3730     3767      +37     
  Misses          898      898              
- Partials        171      174       +3

Impacted Files	Coverage Δ	Complexity Δ
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`93.27% <92.5%> (-0.4%)`	`12 <1> (+1)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b3357c...1e29a4e. Read the comment docs.

codecov · 2019-06-28T09:40:20Z

Codecov Report

Merging #746 into master will increase coverage by 0.19%.
The diff coverage is 91.42%.

@@             Coverage Diff              @@
##             master     #746      +/-   ##
============================================
+ Coverage     78.19%   78.38%   +0.19%     
- Complexity     1100     1106       +6     
============================================
  Files           197      198       +1     
  Lines          4825     4886      +61     
  Branches        380      385       +5     
============================================
+ Hits           3773     3830      +57     
- Misses          886      887       +1     
- Partials        166      169       +3

Impacted Files	Coverage Δ	Complexity Δ
.../java/com/google/api/gax/batching/BatcherImpl.java	`97.9% <91.42%> (-2.1%)`	`15 <1> (+1)`
.../google/api/gax/batching/BatchingCallSettings.java	`96.15% <0%> (ø)`	`5% <0%> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0de16d5...bb437b0. Read the comment docs.

igorbernstein2 · 2019-08-01T18:17:37Z

+          Level level = Level.SEVERE;
+          if (LOG.isLoggable(level)) {
+            String message =
+                "*~*~*~ Batcher was not closed properly!!! ~*~*~*"


Please remove ascii art

igorbernstein2 · 2019-08-01T18:23:13Z

+            LogRecord lr = new LogRecord(level, message);
+            lr.setLoggerName(LOG.getName());
+            lr.setThrown(maybeAllocationSite);
+            LOG.log(lr);


why not call LOG.log(level, message, maybeAllocationSite).

Also why is it a maybe?

igorbernstein2

LGTM.
@vam-google please take a lok

vam-google · 2019-08-12T20:53:42Z

  private final UnaryCallable<RequestT, ResponseT> unaryCallable;
  private final RequestT prototype;
  private final BatchingSettings batchingSettings;
+  private final BatcherReference phantom;


It looks like most of this PR mimics the grpc's implementation of ManagedChannelReference, so that is I'm not questioning the stylistic things in this PR (some of them are questionable, but for sake of consistency we can keep them).

But one small thing anyways. This variable is called phantom (same in grpc, so "consistent"). At the same time, the actual type of the referrence is WeakReferrence. Since JDK has PhantomReference as well, I think calling this field "phantom" is incorrect (maybe historically in grpc implementation it used to be PhantomReference, and then was changed to WearkReference without renaming the actual variable). Please rename it to something, which does not make wrong impression on it (that it is a phantom referrence instead of weak).

Thanks for the feedback. I updated the BatcherReference variable name to `currentBatcherReference'.

vam-google · 2019-08-12T20:56:42Z

+      if (actualRemaining == 0) {
+        break;
+      }
+      Thread.sleep(100L * (1L << retry));


How much time does this (and another ThreadSleep in this PR) add to tests execution time of gax tests? Sometimes it is very hard to avoid adding sleeps in tests so they behave predictably, but we should do it only if there is no other practical way, and if we do so, ideally the sleeps should sleep for as little as possible (while still keeping the tests stable and not flaky).

Is sleep necessary here? What is the minimum sleep time, which still keeps the tests stable?

It should be minimal

The BatcherImplTest contains 3 Thread#sleep, All of them are under the retry loop. These tests are being completed with in first attempt in my local workstation. All three Thread#sleep test cases are taking ~100ms, and the total time to finish the BatcherImplTest is ~320ms(in my local machine) I hope this should be fine.

Is sleep necessary here?

yes, we do need this sleeps interval to trigger auto flush & to let GC collect the garbage batcher instances.

What is the minimum sleep time, which still keeps the tests stable?

We had added 100ms as initial sleep time, but after checking it seems 50ms works fine as well, so changed the initial sleep time to 50ms.

igorbernstein2

Please rebase for #771

Displays a warning in the console when Batchers are garbage collected without calling to Batcher#close(). Derived from [ManagedChannelImpl](https://github.com/grpc/grpc-java/blob/48ca4527c14a95914f9cb7f58ec72997cb96899a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java#L1262). - Addressing feedback for logging messages - Rebase with current master - Minimize the sleep time in the test case

igorbernstein2 · 2019-08-14T14:44:50Z

+        new ConcurrentHashMap<>();
+
+    private static final String ALLOCATION_SITE_PROPERTY_NAME =
+        "com.google.api.gax.batching.v2.Batcher.enableAllocationTracking";


please update the v2 name

igorbernstein2

LGTM, but please update the string reference to v2

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 27, 2019

igorbernstein2 suggested changes Jun 27, 2019

View reviewed changes

rahulKQL force-pushed the gc_warn branch from 1e29a4e to 71c3c46 Compare August 1, 2019 15:51

rahulKQL requested a review from igorbernstein2 August 1, 2019 15:56

igorbernstein2 reviewed Aug 1, 2019

View reviewed changes

igorbernstein2 approved these changes Aug 5, 2019

View reviewed changes

igorbernstein2 requested a review from vam-google August 5, 2019 13:44

igorbernstein2 assigned vam-google Aug 5, 2019

vam-google reviewed Aug 12, 2019

View reviewed changes

igorbernstein2 suggested changes Aug 13, 2019

View reviewed changes

rahulKQL force-pushed the gc_warn branch from 0798622 to f870cd4 Compare August 14, 2019 07:24

rahulKQL requested a review from igorbernstein2 August 14, 2019 07:37

igorbernstein2 reviewed Aug 14, 2019

View reviewed changes

igorbernstein2 approved these changes Aug 14, 2019

View reviewed changes

Removing v2 reference from Batcher Allocation site property

bb437b0

igorbernstein2 merged commit e5cd293 into googleapis:master Aug 16, 2019

This was referenced Aug 16, 2019

Release gax-java v1.48.0 #776

Closed

Release gax-java v1.48.0 #780

Merged

Conversation

rahulKQL commented Jun 27, 2019

What this PR contains

Uh oh!

rahulKQL commented Jun 27, 2019

Uh oh!

rahulKQL commented Jun 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jun 28, 2019

Codecov Report

Uh oh!

codecov Bot commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jun 28, 2019 •

edited

Loading