Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document message auto-leasing, make it optional #115

Closed
toots opened this issue Mar 29, 2018 · 8 comments
Closed

Document message auto-leasing, make it optional #115

toots opened this issue Mar 29, 2018 · 8 comments
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: enhancement

Comments

@toots
Copy link

toots commented Mar 29, 2018

We've been hit very hard by the duplicate messages issues recently reported (#83, #88, #99). I was even more surprised to learn about an un-documented auto-lease feature where message's ack deadline is extended while the processing function is running (#81).

Please, document this feature. Most developper will expect an API client to stick as close as possible to the semantics of the underlying API. An advanced feature such as this one absolutely needs to be documented, otherwise many developers will get confused.

Second, please make it optional. This is, in my opinion, a feature that should be left up to the developper to implement, according to its application logic.

@callmehiphop
Copy link
Contributor

@toots I'm sorry to hear you've run into some issues!

Please, document this feature.

This is a failure on our part and you're absolutely right. I'm going to make this a priority for myself within the next week. I apologise for any confusion the lack of documentation has caused.

Second, please make it optional.

This isn't the first time we've seen a request like this. Originally this was implemented at the recommendation of the PubSub team. If you don't mind me asking, is there something specific you don't like about this (or any) feature? Any input is welcome, our goal is to make the API as simple to use as possible and it sounds like we might not be meeting your expectations.

If you find that you want a finer level of control than what the client currently offers, I would encourage you to look at our GAPIC layer. It's a generated client that mirrors the semantics almost perfectly, however you'll find it lacks some of the conveniences we try and offer in the hand written layer.

@callmehiphop callmehiphop added priority: p2 Moderately-important priority. Fix may not be included in next release. type: enhancement labels Mar 29, 2018
@tedj
Copy link

tedj commented Mar 29, 2018 via email

@toots
Copy link
Author

toots commented Mar 29, 2018

Hi @callmehiphop and thanks for your response.

The issue for me is that, when developing my application, I rely on the documentation to understand how the code is supposed to work and to make assumptions about what might be going on when I see issues. In the case at hand, I was trying to understand why I was seeing duplicate messages being processed while under my expected ack deadline. Knowing how the internals of the library work helps be pinpoint the right places to look at for possible issues.

Secondly, while I understand that a high-level feature is great for simple use, I believe that it's important to also give access to lower-level APIs and functions, i.e. complex things should be possible.

I don't take issues with bugs happening in the auto-leasing feature -- that kind of asynchronous problem is hard to fix, specially if you have no specifications about the underlying application. However, it bothers me that I cannot do without it.

If and when an issue pops out, having a lower-level access to API allows the library's users to circumvent the complex part and work out their own solution. This is particularly true if, on the other hand, I have a good knowledge of my application's logic, for instance expected message processing times. Then I can definitely set up my own lease increase, according to whatever monitoring I have of my message's processing.

@toots
Copy link
Author

toots commented Mar 29, 2018

Sorry, forgot to mention the lower-level clients. I'm using them and they are close to what I would expect for the lower-level API for sure. However, they are not very well documented yet. For instance, when sending batch publish calls, what are the limitations on each batch? Max number of messages and size etc..?

@callmehiphop
Copy link
Contributor

@kir-titievsky @alexander-fenster @jonparrott What are your feelings on allowing users to opt out of auto-leasing? Its been brought up before but usually gets swept under the rug.

Also in regard to better auto-gen docs, I assume this would mean better proto file docs? Would that fall on the API team?

@stephenplusplus stephenplusplus removed the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 2, 2018
@kir-titievsky
Copy link

kir-titievsky commented Apr 2, 2018 via email

@callmehiphop callmehiphop added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 12, 2018
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Sep 25, 2018
@callmehiphop callmehiphop self-assigned this Nov 12, 2018
@callmehiphop
Copy link
Contributor

We have a release coming up in the next several days that will include a new option allowing you to configure/opt out of auto leasing.

const subscription = topic.subscription('my-sub', {
  flowControl: {
    maxExtension: 120 // release lock after 2 minutes
  }
});

Setting maxExtension to 0 is essentially the same as opting out.

@kir-titievsky
Copy link

kir-titievsky commented Jan 15, 2019 via email

@google-cloud-label-sync google-cloud-label-sync bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Jan 31, 2020
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020
feywind pushed a commit to feywind/nodejs-pubsub that referenced this issue Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: enhancement
Projects
None yet
Development

No branches or pull requests

7 participants