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

Concurrent ios pushes #497

Merged
merged 14 commits into from
Apr 23, 2020
Merged

Concurrent ios pushes #497

merged 14 commits into from
Apr 23, 2020

Conversation

McPo
Copy link
Contributor

@McPo McPo commented Apr 21, 2020

Runs client.Push concurrently for iOS pushes, drastically increases performance of gorush. I have tested this along with ensuring logs are return in sync mode and that retries are made. It all seems ok. (Although maybe Im missing something??)

Resolves (hopefully) #470

McPo added 3 commits April 21, 2020 16:11
…bout "loop variable token captured by", need to determine if its a legitiment issue, as the lint message has now disappeared
@McPo
Copy link
Contributor Author

McPo commented Apr 21, 2020

It appears that tests are failing due to the redis service not running

=== RUN   TestRedisServerError
556	2020/04/21 16:32:48 Can't connect redis server: dial tcp 192.168.240.2:6370: connect: connection refused
557	--- PASS: TestRedisServerError (0.00s)
558	=== RUN   TestRedisEngine
559	--- PASS: TestRedisEngine (0.00s)
560	PASS
561	coverage: 100.0% of statements
562	ok  	github.com/appleboy/gorush/storage/redis	0.011s	coverage: 100.0% of statements
563	FAIL

yaronius
yaronius previously approved these changes Apr 21, 2020
Copy link
Contributor

@slimus slimus left a comment

Choose a reason for hiding this comment

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

Is it possible to write some bench tests for Ios send push case?

gorush/notification_apns.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yaronius
Copy link
Contributor

yaronius commented Apr 22, 2020

It appears that tests are failing due to the redis service not running

It seems to me that Redis is OK and the problem with tests is in the Makefile:

make: *** [Makefile:98: test] Error 1

P.S. Nope, my bad, Makefile is OK as well... Strange...

@slimus
Copy link
Contributor

slimus commented Apr 22, 2020

@McPo @yaronius don't worry about tests.

@McPo
Copy link
Contributor Author

McPo commented Apr 22, 2020

Is it possible to write some bench tests for Ios send push case?

How would you want such bench test to be done? Would you want it automated with an assert, or just report. Would a separate test suite maybe be more appropriate?

Say mock out the the client.push to sleep for 1 seconds. Have it send 10 notifications, check if it return less than 2 seconds?

The thing is, we are then basically testing that the co-routine is running in parallel. Which is another reason I was using the external library to limit it, It could then have been argued we were basically just re-testing library code.

I don't think theres a massive benefit to adding automated benchmarks tests. Id suggest maybe adding something similar to @yaronius script, maybe running it in sync mode and using time.

gorush/notification_apns.go Outdated Show resolved Hide resolved
slimus
slimus previously approved these changes Apr 22, 2020
Copy link
Contributor

@slimus slimus left a comment

Choose a reason for hiding this comment

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

lgtm

yaronius
yaronius previously approved these changes Apr 22, 2020
@McPo McPo dismissed stale reviews from yaronius and slimus via 20c25db April 22, 2020 16:00
@McPo
Copy link
Contributor Author

McPo commented Apr 22, 2020

I thought PushToIOS would've been queued, ie If an incoming push was being processed, if another incoming push was made it would wait till the current one had finished. This isn't the case, as such the MaxConcurrentPushes was only being enforce on a per incoming push basis.

So Ive since made it that MaxConcurrentPushes applies across all pushes by moving the channel to global.

One caveat with this is, a new push will be processed with the same priority as the previous one, ie they will be mixed in. As opposed to finishing the first push and then moving one. Its neither correct nor incorrect.

I think thats us now.

@appleboy
Copy link
Owner

@McPo Please fix the error: https://cloud.drone.io/appleboy/gorush/322/1/8

@McPo
Copy link
Contributor Author

McPo commented Apr 22, 2020

@McPo Please fix the error: https://cloud.drone.io/appleboy/gorush/322/1/8

Done! Sorry about that.
Looks better now, tests still failing due to redis though.

@appleboy
Copy link
Owner

fail in TestPushToIOS func.

README.md Outdated Show resolved Hide resolved
McPo and others added 2 commits April 22, 2020 20:52
Co-Authored-By: Yaroslav "Zorg" Zborovsky <yaronius@users.noreply.github.com>
config/config.go Outdated Show resolved Hide resolved
@McPo McPo requested a review from appleboy April 23, 2020 10:08
@appleboy appleboy merged commit 685a87c into appleboy:master Apr 23, 2020
@appleboy
Copy link
Owner

@McPo Thanks.

@appleboy appleboy added this to the v1.13.0 milestone Apr 27, 2020
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.

4 participants