Logs warning message when Batcher is not closed on GC #746
Conversation
|
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 |
|
@igorbernstein2 Please have a look at this change. |
gax/src/test/java/com/google/api/gax/batching/v2/BatcherImplTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/batching/v2/BatcherImplTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/batching/v2/BatcherImplTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/batching/v2/BatcherImplTest.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| Level level = Level.SEVERE; | ||
| if (LOG.isLoggable(level)) { | ||
| String message = | ||
| "*~*~*~ Batcher was not closed properly!!! ~*~*~*" |
There was a problem hiding this comment.
Please remove ascii art
gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java
Outdated
Show resolved
Hide resolved
| LogRecord lr = new LogRecord(level, message); | ||
| lr.setLoggerName(LOG.getName()); | ||
| lr.setThrown(maybeAllocationSite); | ||
| LOG.log(lr); |
There was a problem hiding this comment.
why not call LOG.log(level, message, maybeAllocationSite).
Also why is it a maybe?
gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java
Outdated
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java
Outdated
Show resolved
Hide resolved
| private final BatchingDescriptor<ElementT, ElementResultT, RequestT, ResponseT> | ||
| batchingDescriptor; | ||
| private final UnaryCallable<RequestT, ResponseT> unaryCallable; | ||
| private final RequestT prototype; | ||
| private final BatchingSettings batchingSettings; | ||
| private final BatcherReference phantom; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Thanks for the feedback. I updated the BatcherReference variable name to `currentBatcherReference'.
| if (actualRemaining == 0) { | ||
| break; | ||
| } | ||
| Thread.sleep(100L * (1L << retry)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It should be minimal
There was a problem hiding this comment.
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.
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
| new ConcurrentHashMap<>(); | ||
|
|
||
| private static final String ALLOCATION_SITE_PROPERTY_NAME = | ||
| "com.google.api.gax.batching.v2.Batcher.enableAllocationTracking"; |
There was a problem hiding this comment.
please update the v2 name
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.