PubSub : Fix Publisher.shutdown should return promptly #4956

abhinav-qlogic · 2019-04-16T12:16:44Z

codecov · 2019-04-16T12:56:02Z

Codecov Report

Merging #4956 into master will increase coverage by 0.03%.
The diff coverage is 69.23%.

@@             Coverage Diff              @@
##             master    #4956      +/-   ##
============================================
+ Coverage     50.33%   50.36%   +0.03%     
+ Complexity    23676    23667       -9     
============================================
  Files          2238     2233       -5     
  Lines        226057   225866     -191     
  Branches      24961    24959       -2     
============================================
- Hits         113775   113750      -25     
+ Misses       103678   103518     -160     
+ Partials       8604     8598       -6

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/google/cloud/pubsub/v1/Publisher.java	`80.93% <69.23%> (-4.6%)`	`28 <2> (ø)`
...google/cloud/bigtable/admin/v2/models/GCRules.java	`55.66% <0%> (-13.21%)`	`12% <0%> (-4%)`
...ge/contrib/nio/CloudStorageFileSystemProvider.java	`61.26% <0%> (-0.47%)`	`70% <0%> (-1%)`
...n/java/com/google/cloud/bigquery/BigQueryImpl.java	`83.83% <0%> (-0.26%)`	`52% <0%> (-1%)`
...ud/storage/contrib/nio/testing/FakeStorageRpc.java	`63.54% <0%> (-0.18%)`	`40% <0%> (-1%)`
...cloud/pubsub/v1/StreamingSubscriberConnection.java	`64.75% <0%> (ø)`	`9% <0%> (ø)`	⬇️
...loud/compute/v1/InsertNodeTemplateHttpRequest.java	`23.92% <0%> (ø)`	`9% <0%> (ø)`	⬇️
...in/java/com/google/cloud/compute/v1/NodeGroup.java	`38.6% <0%> (ø)`	`17% <0%> (ø)`	⬇️
...java/com/google/cloud/compute/v1/NodeTemplate.java	`36.41% <0%> (ø)`	`19% <0%> (ø)`	⬇️
... and 20 more

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 ac0084f...d792545. Read the comment docs.

sduskis · 2019-04-16T15:08:11Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

+      for (AutoCloseable closeable : closeables) {
+        ExecutorAsBackgroundResource executorAsBackgroundResource =
+            (ExecutorAsBackgroundResource) closeable;
+        isAwaited = executorAsBackgroundResource.awaitTermination(duration, unit);


The user is asking for something like: wait for 8 seconds total for termination.

publisherStub.awaitTermination(duration, unit); could take 2.1 seconds. The next await termination should wait 8-2.1 seconds.

Please keep track of the start time, and the number of millis that have gone by since the method was invoked, and then wait the remainder of the users requested timeout.

@sduskis changes done

sduskis · 2019-04-16T15:17:14Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

    for (AutoCloseable closeable : closeables) {
-      closeable.close();
+      ExecutorAsBackgroundResource executorAsBackgroundResource =


closable.close() calls shutdown() under the covers. There's no need to cast to a ExecutorAsBackgroundResource

@sduskis changes done

sduskis · 2019-04-18T14:01:25Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

+    long remainingDuration = TimeUnit.MILLISECONDS.convert(duration, unit);
+    messagesWaiter.waitNoMessages();
+    remainingDuration = getRemainingDuration(remainingDuration, startDuration);
+    startDuration = System.currentTimeMillis();


Please don't update startDuration

sduskis · 2019-04-18T14:01:37Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

@@ -430,7 +429,32 @@ public void shutdown() throws Exception {
   * <p>Call this method to make sure all resources are freed properly.
   */
  public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
-    return publisherStub.awaitTermination(duration, unit);
+    long startDuration = System.currentTimeMillis();


please make startDuration final. There's never a need to update it, since startDuration represents the start time when the method was called.

@sduskis startDuration needs to be update because it is used(counting) to find elapsed time which occur to execute waitNoMessages method only, so we have to minus this time from actual time pass to duration parameter of awaitTermination method and so on needs to update every time to find elapsed time so we can remove it from total remaining time.

I think I understand what you mean. Let's have a static startTime and totalDurationMs variables that are final. We can use those plus current time to calculate remainingDurationMs

@sduskis chnages done

sduskis · 2019-04-25T16:24:10Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

@@ -430,7 +429,37 @@ public void shutdown() throws Exception {
   * <p>Call this method to make sure all resources are freed properly.
   */
  public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
-    return publisherStub.awaitTermination(duration, unit);
+    final long startDuration = System.currentTimeMillis();


Please call this variable startTimeMs

sduskis · 2019-04-25T16:24:51Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

+        ExecutorAsBackgroundResource executorAsBackgroundResource =
+            (ExecutorAsBackgroundResource) closeable;
+        remainingDuration = getRemainingDuration(startDuration, totalDurationMs);
+        System.out.println(remainingDuration);


please remove this println.

sduskis · 2019-04-25T16:25:34Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

+          return false;
+        }
+      }
+    } else {


please remove the else.

sduskis · 2019-04-25T16:25:50Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

+    } else {
+      return false;
+    }
+    return true;


please use return isAwaited

@sduskis changes done

…ngPublish

sduskis

It looks like you had a bad merge here. There are some changes that your PR reverted. Please update the PR to only change the thins you intended to change.

…n method" This reverts commit 4bc01c7

…-3687

ajaaym · 2019-04-30T11:41:16Z

@abhinav-qlogic closing this PR due to lot of unintended merge/conflict. Please create new with just your change.

abhinav-qlogic requested a review from a team as a code owner April 16, 2019 12:16

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

pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2019

sduskis suggested changes Apr 16, 2019

View reviewed changes

pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 18, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 18, 2019

sduskis suggested changes Apr 18, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label Apr 23, 2019

sduskis suggested changes Apr 25, 2019

View reviewed changes

abhinav-qlogic added 7 commits April 29, 2019 12:00

fix for publisher shutdown return promtly

c11c26f

fix for publisher shutdown return promtly

016061f

fix for publisher shutdown and awaitTermimation test case added

5380025

fix for publisher shutdown and awaitTermimation test case added

e34b028

fix for publisher shutdown review changes

41365ef

fix for publisher shutdown review changes

35ee213

fix for publisher shutdown review changes for awaitTermination method

4bc01c7

abhinav-qlogic force-pushed the api-pubsub-3687 branch from bb5718c to 4bc01c7 Compare April 29, 2019 07:15

pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019

abhinav-qlogic added 2 commits April 29, 2019 16:28

fix messageSize missing field added in OutstandingPublish

32ae2f3

fix changes reverted for messageSize missing field added in Outstandi…

155ca32

…ngPublish

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019

sduskis suggested changes Apr 29, 2019

View reviewed changes

abhinav-qlogic added 2 commits April 30, 2019 17:01

Revert "fix for publisher shutdown review changes for awaitTerminatio…

c5ae0a2

…n method" This reverts commit 4bc01c7

Merge remote-tracking branch 'origin/api-pubsub-3687' into api-pubsub…

d792545

…-3687

ajaaym closed this Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PubSub : Fix Publisher.shutdown should return promptly #4956

PubSub : Fix Publisher.shutdown should return promptly #4956

abhinav-qlogic commented Apr 16, 2019

codecov bot commented Apr 16, 2019 •

edited

Loading

sduskis Apr 16, 2019

abhinav-qlogic Apr 18, 2019

sduskis Apr 16, 2019

abhinav-qlogic Apr 18, 2019

sduskis Apr 18, 2019

sduskis Apr 18, 2019

abhinav-qlogic Apr 18, 2019

sduskis Apr 18, 2019

abhinav-qlogic Apr 25, 2019

sduskis Apr 25, 2019

sduskis Apr 25, 2019

sduskis Apr 25, 2019

sduskis Apr 25, 2019

abhinav-qlogic Apr 29, 2019

sduskis left a comment

ajaaym commented Apr 30, 2019

PubSub : Fix Publisher.shutdown should return promptly #4956

PubSub : Fix Publisher.shutdown should return promptly #4956

Conversation

abhinav-qlogic commented Apr 16, 2019

codecov bot commented Apr 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sduskis left a comment

Choose a reason for hiding this comment

ajaaym commented Apr 30, 2019

codecov bot commented Apr 16, 2019 •

edited

Loading