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

Fix for _PushStatus Stuck 'running' when Count is Off #4319

Merged
merged 3 commits into from
Nov 5, 2017

Conversation

montymxb
Copy link
Contributor

@montymxb montymxb commented Nov 4, 2017

This is a proposed fix for #4315, where count computed for tracking outstanding pushes can differ from the actual # of devices that will be sent to.

During large operations this can occur where there is a significant time variation between the original computing of count and completion of the pushes. Also count can potentially provide an incorrect value on sharded dbs. With this in mind it makes more sense to track the batches instead. The # of batches is computed from the count that is set on _PushStatus, and even if the number of matched _Installation objects changes this does not. Given that, you can reliably track the batches instead of the count and still have an accurate running time.

This does not modify the existing behavior with count, so any dependence that anything or anyone may have on that factor is unchanged. The tracking for batches is achieved via an additional batches field on _PushStatus.

This adds some tests with a modified push adapter that either 'drops' or 'adds' installations during send ops to simulate count being inaccurate.

@codecov
Copy link

codecov bot commented Nov 4, 2017

Codecov Report

Merging #4319 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4319      +/-   ##
==========================================
+ Coverage    92.5%   92.52%   +0.02%     
==========================================
  Files         118      118              
  Lines        8256     8256              
==========================================
+ Hits         7637     7639       +2     
+ Misses        619      617       -2
Impacted Files Coverage Δ
src/Controllers/SchemaController.js 97.09% <ø> (ø) ⬆️
src/Push/PushQueue.js 100% <100%> (ø) ⬆️
src/StatusHandler.js 99.29% <100%> (+0.7%) ⬆️
src/RestWrite.js 93.57% <0%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 842343a...366ba8a. Read the comment docs.

@montymxb montymxb requested a review from flovilmart November 4, 2017 17:18
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

I would completely remove the count as in the end, it’s useless to keep it as a count, perhaps use the number batch inside the count field instead. This way we don’t need to change the schema either.

@montymxb
Copy link
Contributor Author

montymxb commented Nov 4, 2017

I originally used just count but was unsure if there was any referencing code on the dashboard or somewhere else that it might goof with. I can change that back if there's nothing like that.

@flovilmart
Copy link
Contributor

Nope no references whatsoever. The scheduled state is not handled in the dashboard though:

@montymxb
Copy link
Contributor Author

montymxb commented Nov 4, 2017

CI failed with the following:

  • server has createLiveQueryServer

    • Failed: schema class name does not revalidate
  • ParseLiveQueryServer can handle connect command

    • Failed: cannot perform operation: a background operation is currently running for collection parse._User
  • Installations update android device token with duplicate device token

    • Expected 1 to equal 0.

Local looked fine so I'm rerunning to see if it clears.

@montymxb
Copy link
Contributor Author

montymxb commented Nov 4, 2017

@flovilmart alright tests look good 👍

@flovilmart
Copy link
Contributor

Looks great thanks!

@flovilmart flovilmart merged commit c1a7347 into parse-community:master Nov 5, 2017
@montymxb montymxb deleted the push-stuck-running branch November 5, 2017 18:43
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
…y#4319)

* Fix for _PushStatus stuck 'running' if count is off

* use 'count' for batches

* push worker test fix
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.

2 participants