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

Shared concurrent heartbeat() promise #1026

Merged
merged 7 commits into from
Mar 8, 2021
Merged

Conversation

t-d-d
Copy link
Contributor

@t-d-d t-d-d commented Feb 12, 2021

All concurrent heartbeat() calls will return the same promise object. Proposed solution to #1025

Added a test that reproduces #1025 but more tests needed.

@t-d-d t-d-d marked this pull request as ready for review February 14, 2021 07:56
Copy link
Owner

@tulios tulios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-d-d thanks a lot for the PR. I left some comments. They are basically suggestions; the only one that I would like to see changed is making the utility a little less dense, so it's easier to grasp.

* @param { () => Promise<T> } [defaultFunc]
* @returns { (func?: () => Promise<T>) => Promise<T> }
*/
module.exports = defaultFunc => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need defaultFunc? It's currently not used anywhere. I would also use curly braces and make this a little bit less dense, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right - it is trying to be too clever. I had both options because it's slightly easier to use the 'invocation time' method from a class (well, less code changes are needed to adopt it.)

I have changed to only provide option at initialisation time. Let me know what you think.

src/consumer/consumerGroup.js Outdated Show resolved Hide resolved
src/consumer/consumerGroup.js Outdated Show resolved Hide resolved
@t-d-d
Copy link
Contributor Author

t-d-d commented Feb 16, 2021

Actually this shouldn't be merged until we conclude the issue of what to do with failed heartbeats that @Nevon raises in #1025 .
My thinking is that there's no point in queuing up heartbeat calls that will get issued one-by-one when they fail. I presume that in most cases, if one fails they are all going to fail.

@t-d-d t-d-d requested a review from tulios February 16, 2021 13:44
@tulios tulios merged commit 958eda1 into tulios:master Mar 8, 2021
@t-d-d t-d-d deleted the shared_heartbeat branch March 20, 2021 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants