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

Make use of Guzzle Pool to improve efficiency #401

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

Gugu7264
Copy link
Contributor

@Gugu7264 Gugu7264 commented Jun 17, 2024

Fixes #367, fixes #195.

Since making use of the Pool refrains from returning anything to the user, a callback argument is added in the new flushPooled function, which will get called for each response received.

It was mentioned in #367 to transform the prepare method into a Generator, but that's not possible if we want to be able to return the Request alongside the Response.

@Minishlink
Copy link
Member

Minishlink commented Jun 18, 2024

Thanks @Gugu7264 for the PR and your help on the other issues.

  1. Is there any advantage to use flush instead of flushPooled? Was flushPooled added to prevent a breaking change? If so, next version will have several breaking changes so we could group them together.
  2. Apparently some tests are failing
  3. Can you add a test for when the user chooses flushPooled instead of flush please? (depending on the answer of 1 ;) )

@Gugu7264
Copy link
Contributor Author

It's indeed added as a separate function because of the breaking change behavior of using a callback instead if returning/yielding something. Apart from that, I do not think there's any advantage.
Will fix tests and add tests

@Gugu7264
Copy link
Contributor Author

  1. Apparently some tests are failing
  2. Can you add a test for when the user chooses flushPooled instead of flush please? (depending on the answer of 1 ;) )

@Minishlink Here are 2 commits that should fix 2. and 3. I haven't done community PHP development before and wasn't aware of the tests set up in composer, hence the failing (I was missing a comma...).
I've now run the tests, apart from test:unit (but I ran test:unit_offline) so everything should be good now :)

@Gugu7264
Copy link
Contributor Author

  1. Is there any advantage to use flush instead of flushPooled? Was flushPooled added to prevent a breaking change? If so, next version will have several breaking changes so we could group them together.

As for this, I added this as separate since I don't know when will next version come, but I can change it for the next breaking version? (maybe there should be a separate branch for the breaking version?)

Copy link
Member

@Minishlink Minishlink left a comment

Choose a reason for hiding this comment

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

Let's make this a non breaking change for the moment since it changes some things like error handling etc

Can you update the readme please? In the "how do I scale" section, add a line for flushPooled

src/WebPush.php Outdated Show resolved Hide resolved
@Gugu7264
Copy link
Contributor Author

Here's a fixed version @Minishlink

Copy link
Member

@Minishlink Minishlink 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!

@Minishlink Minishlink merged commit b33c70d into web-push-libs:master Jun 18, 2024
9 checks passed
@Gugu7264 Gugu7264 deleted the pooled-flush branch June 18, 2024 16:34
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.

Consider using GuzzleHttp\Pool in flush Optimize for thousands of subscribers
2 participants