Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Added redeliver_timeout and claim_interval options #12976

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

Merged
merged 1 commit into from
Oct 23, 2020
Merged

Added redeliver_timeout and claim_interval options #12976

merged 1 commit into from
Oct 23, 2020

Conversation

toooni
Copy link
Contributor

@toooni toooni commented Jan 23, 2020

Adds new optional config parameters introduced by symfony/symfony#35384

There should never be two or more `messenger:consume` commands running with the same
config (stream, group and consumer name) to avoid having a message handled more than once.
Using the ``HOSTNAME`` as the consumer might often be a good idea. In case you are using
Kubernetes to orchestrate your containers, consider using a ``StatefulSet``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this is what @Toflar wanted to document in #11869. ping @Toflar anything you want to add here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, with this PR it's not really relevant anymore. You can use a regular Deployment which changes hostnames after restarts but the PR is here to fix exactly that. The StatefulSet is just so that you always get the same hostnames back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah thats correct still think its good to have the hostname which doesn't change as it avoids spaming redis with new consumers as they recommend to reuse consumer names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then that should probably be mentioned here but other than that, there's nothing to add regarding k8s :)

@OskarStark OskarStark added the Waiting Code Merge Docs for features pending to be merged label Jan 28, 2020
@toooni toooni changed the base branch from 4.4 to master February 3, 2020 15:26
fabpot added a commit to symfony/symfony that referenced this pull request Feb 8, 2020
…is) (toooni)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Messenger] Add receiving of old pending messages (redis)

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | symfony/symfony-docs#12976

This PR makes it possible for the redis transport to get abandoned messages from not running/idle consumers by using `XPENDING` and `XCLAIM`.
Usually it would be best to let the claiming of pending messages be handled by a separate command. Since the messenger component's commands are fixed, we do need to set a `claimTimeout`. The `claimTimeout` defines how long an idle message should be left alone until it will get claimed by the current consumer (Must be a value higher than the longest running handling time of a message or else the message will be handled twice).
Using this solution makes the remarks (symfony/symfony-docs#11869 (review)) regarding not being able to use the hostname as consumer name obsolete. I would even recommend the hostname as the consumer name.

**Questions**

- [x] Which value should we use as default `claimTimeout`?
- [x] How should the `claimTimeout` be configured?
- [x] Feature or Bugfix?

I will create a docs PR and a PR for the other branches as soon as the questions are resolved.

Commits
-------

9cb6fdf Implemted receiving of old pending messages
@HeahDude HeahDude removed the Waiting Code Merge Docs for features pending to be merged label Feb 19, 2020
@HeahDude HeahDude added this to the 5.1 milestone Feb 19, 2020
@HeahDude HeahDude requested a review from weaverryan February 19, 2020 20:13
@wouterj
Copy link
Member

wouterj commented Oct 21, 2020

@Nyholm can you have a look at this PR please?

messenger.rst Outdated
================== ===================================== =========================

.. caution::

There should never be two or more `messenger:consume` commands running with the same
Copy link
Member

Choose a reason for hiding this comment

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

There should never be more than one messenger:consume...

@Nyholm
Copy link
Member

Nyholm commented Oct 23, 2020

I think this looks good. It needs a rebase and syntax fix on the first column in the table

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

I am happy with these changes.

@wouterj wouterj changed the base branch from master to 5.1 October 23, 2020 13:04
wouterj added a commit that referenced this pull request Oct 23, 2020
@wouterj wouterj merged commit b485a8d into symfony:5.1 Oct 23, 2020
@wouterj
Copy link
Member

wouterj commented Oct 23, 2020

Thank you @toooni! I'm sorry for the long wait, there has been a great lack of Messenger knowledge in the doc reviewer time for a long while :( At last, this is now merged in 5.1 (and I'll merge it into 5.2 from there).
Congratulations with your first merged doc PR!

@Nyholm
Copy link
Member

Nyholm commented Oct 23, 2020

Wooho. Congratulations!

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Oct 23, 2020
* 5.1:
  [symfony#12976] Added the new options to the versionadded directive
  [symfony#12976] Minor syntax fix
  Added redeliver_timeout and claim_interval options
  [Security] Remove extra argument from call to EntityManager#flush()
@toooni toooni deleted the claim_abandoned_messages_44 branch October 23, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.