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

Adds resume functionality #33

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

mvrsss
Copy link
Contributor

@mvrsss mvrsss commented Jan 11, 2024

Problem statement:

  • Previously, there was no resume() function that would restart the consumer after cancellation.

Proposed changes:

  • Implements resume() function
  • Adds CANCELLING_BUT_RESUMING state to the consumer
  • CANCELLING_BUT_RESUMING state is added to support prefetch update in the future
  • Supports timeout in cancel and resume functions

image

Problem statement:
- Previously, there was no resume() function that would restart the consumer after cancellation.

Proposed changes:
- Implements resume() function
- Adds CANCELLING_BUT_RESUMING state to the consumer
- CANCELLING_BUT_RESUMING state is added to support prefetch update in the future
Copy link
Contributor

@adamncasey adamncasey left a comment

Choose a reason for hiding this comment

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

Hoping that by commenting here we get doxygen CI passing?

@@ -0,0 +1,3 @@
New state CANCELLING_BUT_RESUMING is introduced to support resume after cancellation

![image](https://github.com/mvrsss/rmqcpp/assets/60746841/0ccd36db-8a7b-4f15-94fd-764d1559a73b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this image in this repo under the docs folder, and add a brief markdown file in there which references it. This README describes internals to the library but so far the examples folder is used for end-users to quickly get started. I think we should keep those purposes separate

Comment on lines +183 to +186
if (d_state == NOT_CONSUMING) {
d_state = CANCELLED;
return method;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Highlighting this change for the PR history (Shynar already discussed this internally). This fixes an issue where calling cancel on a consumer while the consumer isn't actually consuming, for example due to a connection issue, can put it in a stuck state.

@adamncasey
Copy link
Contributor

adamncasey commented Jan 12, 2024

The Doxygen CI step seems to be trying to write to this repo, which is (rightly) failing for a PR coming from a fork. I'm OK to ignore this CI check for now and someone can address the problem with this check separately


- previously cancelled consumer would be restarted after `resume()` call

![image](https://github.com/mvrsss/rmqcpp/assets/60746841/54b025ee-3d7a-48de-bbcf-97ba00abf76b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
![image](https://github.com/mvrsss/rmqcpp/assets/60746841/54b025ee-3d7a-48de-bbcf-97ba00abf76b)
![image](./consumer-state-diagram.png)

Need to commit the image into the git repo, but we could make that a separate PR to facilitate merging this now

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.

3 participants