Add returnImmediately option to PubSub's pullAsync

[mziccard]

This fixes #1157 and #1299

@aozarov @GarrettJones I would like your opinion on the following:

pullAsync throws DEADLINE_EXCEEDED when a timeout is reached (e.g. PubSubOptions.Builder.initialTimeout()). This still applies when the PullOption.returnImmediately(false) option is provided. I have the feeling that this is kind of unexpected as users that provide the PullOption.returnImmediately(false) option might not want pullAsync to be interrupted by timeouts. What do you think? Do you also feel that throwing on timeouts is an unexpected behavior when PullOption.returnImmediately(false) is passed?

@garrettjonesgoogle is there anything we can do at the Veneer Toolkit layer to avoid failing on timeouts when PullRequest.getReturnImmediately() == false?

[coveralls]

Coverage increased (+0.007%) to 84.715% when pulling 30fd965 on mziccard:return-immediately into b61af33 on GoogleCloudPlatform:master.

[aozarov]

I think introducing the concept of returnImmediately for async pulls adds more confusion to the API (e.g. PullOption which is in reality PullOptionAsync, method can sometime return Iterator and sometimes may timeout) with a negligible benefit (as the future could be cancelled when desired).

I think that the issue that pullAsync throws DEADLINE_EXCEEDED when a PubSubOptions.Builder.initialTimeout has reached (hence smaller than the get timeout) is a separate
one and should be addressed independently.

Couldn't we set the timeout to infinite in that particular VTKIT call? If not, then I assume we can catch this failure and pull again?

[mziccard]

I think introducing the concept of returnImmediately for async pulls adds more confusion to the API (e.g. PullOption which is in reality PullOptionAsync, method can sometime return Iterator and sometimes may timeout) with a negligible benefit (as the future could be cancelled when desired).

I agree in principle but this has been requested by several users. Also I don't see what cancelling the future has to to with exposing returnImmediately(false).

I think that the issue that pullAsync throws DEADLINE_EXCEEDED when a PubSubOptions.Builder.initialTimeout has reached (hence smaller than the get timeout) is a separate
one and should be addressed independently.

What is the "get timeout"? initialTimeout is the timeout for the first RPC (timeout is then increased with every retry) so I believe that I am seeing the expected behavior and not a separate issue.

Couldn't we set the timeout to infinite in that particular VTKIT call? If not, then I assume we can catch this failure and pull again?

I am not sure you can set the timeout for only some of VTKIT pull calls, hence I am asking @garrettjonesgoogle. Timing out and retrying a pull that has returnImmediately(false) does the job but could generate much more traffic than what is necessary.

[garrettjonesgoogle]

It is true that you can't set a timeout for a specific instance of a call, but there is a hack available: create a second instead of PublisherApi, sharing the same channel, but with different timeout settings.

I don't know that it makes sense to change the spi layer's semantics for this case. If we wanted to handle the described scenario, we would need special-case logic for populating the correct response when returnImmediately was false (timeout -> empty response) but normal logic when returnImmediately was true (timeout -> exception).

Maybe we should just make it easier to have call-specific timeout overrides.

[aozarov]

I agree in principle but this has been requested by several users. Also I don't see what cancelling the future has to to with exposing returnImmediately(false).

Was it requested because of the timeout issue? Personally, I would create an issue and ask for votes, as personally I see more cons than pros. One could "mimic" returnImmediately(true) by checking Future#isDone and if not (and if desirable) call Future#cancel.

What is the "get timeout"?

Future#get timeout, as I assume this would not be an issue if a user was willing to wait less than the rpc timeout. By 2 issues I mean, 2 orthogonal issues (1) Fix current handling of the RPC layer timeout for async pulls. (2) Add the option for returnImmediatly.

I am not sure you can set the timeout for only some of VTKIT pull calls, hence I am asking @garrettjonesgoogle. Timing out and retrying a pull that has returnImmediately(false) does the job but could generate much more traffic than what is necessary.

Using @garrettjonesgoogle seems fine to me but as I mentioned I think you have another implementation option to deal with this issue.

[mziccard]

One could "mimic" returnImmediately(true) by checking Future#isDone and if not (and if desirable) call Future#cancel.

So are you suggesting that instead of having the PullOption.returnImmediately option we always set returnImmediately = false for pullAsync? I missed this part from your previous comment. Right now we do the exact opposite (i.e. returnImmediately = true unless a MessageConsumer is used). I'd still prefer to offer this in the form of an option instead of favoring one semantics over the other.

Using @garrettjonesgoogle seems fine to me but as I mentioned I think you have another implementation option to deal with this issue.

Future#get timeout and DEADLINE_EXCEEDED timeout are unrelated exceptions. The first is future-related, the second is transport-related. Ideally, DEADLINE_EXCEEDED should not be thrown when returnImmediately = false.
According to what @garrettjonesgoogle said we cannot get rid of DEADLINE_EXCEEDED just for pulls with returnImmediately = false (as veneer toolkit code works now). Setting an infinite timeout for all pulls is not viable as that would also affect synchronous pull with returnImmediately = true, which should instead timeout.

Retrying is an option but that makes the use of returnImmediately = false much less appealing due to the increased number of traffic that would require.

[aozarov]

So are you suggesting that instead of having the PullOption.returnImmediately option we always set returnImmediately = false for pullAsync? I

Yes, that was my original intent for asyncPull (sorry if that was never communicated). However,
I think the synchronous pull should do the opposite (and use returnImmediately = true).

I'd still prefer to offer this in the form of an option instead of favoring one semantics over the other.

OK, I don't like it (for the reasons mentioned above, and bellow), but do what you feel right.

Future#get timeout and DEADLINE_EXCEEDED timeout are unrelated exceptions. The first is future-related, the second is transport-related.

Yes, I get that. Maybe I should not have mentioned it. I was just saying that if you bail your future sooner than the RPC timeout you may never see the RPC timeout.

Ideally, DEADLINE_EXCEEDED should not be thrown when returnImmediately = false.

Ideally, but not necessarily (which is another reason I favored hiding it from async pull).

Setting an infinite timeout for all pulls is not viable as that would also affect synchronous pull with returnImmediately = true, which should instead timeout.

You could use a separate SPI instance just for async pulls (or use the retry as suggested).

Retrying is an option but that makes the use of returnImmediately = false much less appealing due to the increased number of traffic that would require.

Yes, I get your point and that is a price to pay if there is no other way (but I think there is). I also think that when someone is using async pull it is much more likely that they want to wait until there is a message back (or they decide to bail/cancel). What do you think would be the common use-case for a user to do once async pull completes due to an RPC deadline? If you still think that this is an issue and if there is no way to deal with it with a separate settings (e.g. for now use a separate SPI instance) then maybe adding returnImmediatly option (I would have not make it accept boolean but rather use it as a flag to switch from the returnImmediatly=false default mode) as part as specific AsyncPull options would not be as confusing (especially if the option is well documented).
settings

[mziccard]

For background: Pub/Sub can return an empty list of messages when returnImmediately is set to false. This seems to happen when a timeout of ~90seconds is reached on the server side.

This behavior is not documented in the official docs which just state:

If this is specified as true the system will respond immediately even if it is not able to return a message in the subscriptions.pull response. Otherwise the system is allowed to wait until at least one message is available rather than returning no messages. The client may cancel the request if it does not wish to wait any longer for the response.

[mziccard]

This is replaced by #1387