Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
Appearance settings

[Messenger] Implement MessageCountAwareInterface in RedisTransport#39127

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@chrishemmings
Copy link

[Messenger] Implement MessageCountAwareInterface in RedisTransport

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PRn/a

Implement MessageCountAwareInterface in RedisTransport by providing the getMessageCount() method in the Connection class. This calls xLen() on the Redis steam to return the number of items left to process.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

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

could you please add tests?

@jderussejderusse added this to the5.x milestoneNov 20, 2020
Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

This might not be the count we expect. I'm "blocking" this PR until we can confirm it.
See#31545 (comment)
#30917 (comment) for previous attempts.
ping@alexander-schranz

chrishemmings reacted with thumbs up emoji
@chrishemmings
Copy link
Author

chrishemmings commentedNov 21, 2020
edited
Loading

@chalasr I'll check this out. The queue needs to havedelete_after_ack otherwise stream count will be completely incorrect, maybe returnnull for the count ifdelete_after_ack is not set on the connection? I can check some other scenarios around delayed and queued messages as well. Ultimately, I need to get at least a very close approximation as to the number of messages in the queue. Without being able to monitor these numbers, there is not a way I can spot queues that are not processing correctly.

Any pointers would be appreciated. Thanks 👍

@alexander-schranz
Copy link
Contributor

As correctly linked by@chalasr the way to get the length would be what I have commented at:#31545 (comment). The problem here is that redis depending on your redis extension version does return thexinfo indifferent formats so this is not easy to parse to get thelast-delivered-id correctly in all supported versions.

Aslong asdelete_after_ack was set from the beginningxlen would return the correct count but if it was added later to the DSN it and the stream was not manually cleared then it would also not return the correct count for the messages which are expected. So not sure if I would depend onxlen in any case.

@fabpot
Copy link
Member

@chrishemmings Are you still working on this PR?

@chrishemmings
Copy link
Author

@chrishemmings Are you still working on this PR?

Hi there, for now I don't have time to look at this, it's turned into a more complex problem than I initially though. Closing.

@rvanlaak
Copy link
Contributor

To try reviving this issue; would the blocker we had earlier for getting this feature still be there?

Could we use a command like XPENDING for checking the Redis stream length?https://redis.io/commands/xpending/

@chalasr
Copy link
Member

No more blocker, the message count will be available for redis as of Symfony 6.2 thanks to#46229

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jderussejderussejderusse approved these changes

@chalasrchalasrchalasr requested changes

@fabpotfabpotfabpot approved these changes

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

7 participants

@chrishemmings@carsonbot@alexander-schranz@fabpot@rvanlaak@chalasr@jderusse

[8]ページ先頭

©2009-2025 Movatter.jp