Pubsub hp #1483

davidtorres · 2016-12-17T05:03:52Z

No description provided.

googlebot · 2016-12-17T05:03:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

davidtorres · 2016-12-17T05:04:58Z

@pongad please take a look, this adds support for falling back to pull and ack when no streaming pull support and fixes the flakiness on the tests

coveralls · 2016-12-17T06:07:10Z

Changes Unknown when pulling 68d79ad on davidtorres:pubsub-hp into ** on GoogleCloudPlatform:pubsub-hp**.

pongad

Travis and AppVeyor seems to still be flaking but we are moving in the right direction. Thank you for working on this! I have a few comments, mostly stylistic.

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/AbstractSubscriberConnection.java

@@ -142,6 +99,9 @@
    void extendExpiration() {
      expiration = new Instant(clock.millis()).plus(Duration.standardSeconds(nextExtensionSeconds));
      nextExtensionSeconds = 2 * nextExtensionSeconds;
+      if (nextExtensionSeconds > MAX_ACK_DEADLINE_EXTENSION_SECS) {


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/AbstractSubscriberConnection.java

+    }
+    Instant now = new Instant(clock.millis());
+    int totalByteCount = 0;
+    final List<AckHandler> ackHandlers = new ArrayList<>(responseMessages.size());


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/PollingSubscriberConnection.java

+
+    List<List<String>> ackChunks = Lists.partition(acksToSend, MAX_PER_REQUEST_CHANGES);
+    Iterator<List<String>> ackChunksIt = ackChunks.iterator();
+    while (ackChunksIt.hasNext()) {


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/SubscriberImpl.java

-              clock);
-    }
+    streamingSubscriberConnections = new ArrayList<StreamingSubscriberConnection>(numChannels);
+    pollingSubscriberConnections = new ArrayList<PollingSubscriberConnection>(numChannels);


google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/FakeSubscriberServiceImpl.java

-      ImmutableList<String> receivedAcksCopy = ImmutableList.copyOf(acks);
-      acks.clear();
+      List<String> receivedAcksCopy = ImmutableList.copyOf(acks.subList(0, expectedCount));
+      acks.removeAll(receivedAcksCopy);


google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/FakeSubscriberServiceImpl.java

-      modAckDeadlines.clear();
+      List<ModifyAckDeadline> modAckDeadlinesCopy =
+          ImmutableList.copyOf(modAckDeadlines.subList(0, expectedCount));
+      modAckDeadlines.removeAll(modAckDeadlinesCopy);


pongad · 2016-12-19T02:48:13Z

@davidtorres Could you change the commit email? The one you're using, while undoubtedly yours, seems to be running into cla troubles.

pongad · 2016-12-19T02:49:08Z

Also cc @garrettjonesgoogle

googlebot · 2016-12-19T22:02:57Z

CLAs look good, thanks!

davidtorres · 2016-12-19T22:07:19Z

@pongad comments addressed please take a look

davidtorres · 2016-12-19T22:11:34Z

@pongad Not sure about the flakyness, can you point me to where you see this?

coveralls · 2016-12-19T22:36:49Z

Changes Unknown when pulling 0d82897 on davidtorres:pubsub-hp into ** on GoogleCloudPlatform:pubsub-hp**.

google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/FakeSubscriberServiceImpl.java

+      final StreamObserver<StreamingPullResponse> responseObserver) {
+    final Stream stream = new Stream();
+    stream.requestObserver =
+        new StreamObserver<StreamingPullRequest>() {


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/StreamingSubscriberConnection.java

+  @Override
+  void initialize() {
+    final SettableFuture<Void> errorFuture = SettableFuture.create();
+    final ClientResponseObserver<StreamingPullRequest, StreamingPullResponse> responseObserver =


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/AbstractSubscriberConnection.java

+ * Abstract base implementation class of a subscriber connection in charge of receiving subscription
+ * messages.
+ */
+abstract class AbstractSubscriberConnection extends AbstractService {


pongad · 2016-12-19T23:30:38Z

@davidtorres I'll approve the change from my end, please also address Garrett's comments

pongad · 2016-12-19T23:38:35Z

@davidtorres The flake I thought I saw seems to have disappeared 😄

…ss to use containment in favor of inheritance

coveralls · 2016-12-20T22:10:17Z

Changes Unknown when pulling 6ef9df7 on davidtorres:pubsub-hp into ** on GoogleCloudPlatform:pubsub-hp**.

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/MessagesProcessor.java

  }

-  @Override
-  protected void doStop() {
+  protected void stop() {


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/AbstractSubscriberConnection.java

+ * Abstract base implementation class of a subscriber connection in charge of receiving subscription
+ * messages.
+ */
+abstract class AbstractSubscriberConnection extends AbstractService {


garrettjonesgoogle · 2016-12-21T00:18:58Z

LGTM, I don't want to block on more refactoring right now.

coveralls · 2016-12-21T02:33:42Z

Changes Unknown when pulling f8c8dc6 on davidtorres:pubsub-hp into ** on GoogleCloudPlatform:pubsub-hp**.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 17, 2016

pongad suggested changes Dec 19, 2016

View reviewed changes

pongad assigned pongad and garrettjonesgoogle Dec 19, 2016

davidtorres added 2 commits December 19, 2016 17:01

Update

eb693bd

Addressing feedback

0d82897

davidtorres force-pushed the pubsub-hp branch from 68d79ad to 0d82897 Compare December 19, 2016 22:02

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 19, 2016

garrettjonesgoogle reviewed Dec 19, 2016

View reviewed changes

pongad approved these changes Dec 19, 2016

View reviewed changes

davidtorres added 2 commits December 20, 2016 16:16

Addressing the latest comments: mainly refactoring the connection cla…

6ef9df7

…ss to use containment in favor of inheritance

Fixing a race condition on shutdown

6b3c738

garrettjonesgoogle reviewed Dec 21, 2016

View reviewed changes

Addressing last comments of feedback

f8c8dc6

pongad merged commit 61687f0 into googleapis:pubsub-hp Dec 21, 2016

theacodes unassigned pongad Sep 28, 2018

Pubsub hp #1483

Pubsub hp #1483

Conversation

davidtorres commented Dec 17, 2016

googlebot commented Dec 17, 2016

davidtorres commented Dec 17, 2016

coveralls commented Dec 17, 2016

pongad left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

pongad commented Dec 19, 2016

pongad commented Dec 19, 2016

googlebot commented Dec 19, 2016

davidtorres commented Dec 19, 2016

davidtorres commented Dec 19, 2016

coveralls commented Dec 19, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

pongad commented Dec 19, 2016

pongad commented Dec 19, 2016

coveralls commented Dec 20, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Dec 21, 2016

coveralls commented Dec 21, 2016