search

thoughtbot / parity

Shell commands for development, staging, and production parity for Heroku apps
https://thoughtbot.com
MIT License
889 stars 57 forks source link

Replace automatic parallelization of backup jobs with opt-in #176

Closed richiethomas closed 4 years ago

richiethomas commented 4 years ago

Per this issue, right now Parity's default behavior is to attempt to parallelize the pg_restore task across multiple cores. This was causing some parts of the restore op to happen out-of-order, in which case the affected jobs would fail and the restore op as a whole would end unsuccessfully, leaving the DB in a semi-restored state.

This PR removes the default to parallelization, and replaces it with a command line flag which lets the user opt in to parallelization. The default is now to run pg_restore in a single-threaded fashion.

geoffharcourt commented 4 years ago

@richiethomas ack, one last thing, can you replace your commit subject with the PR subject so that the commit log shows what you did? I'll merge as soon as that's updated. I don't want to overwrite your commit as mine.

etcook commented 4 years ago

@richiethomas Thanks for this PR.

richiethomas commented 4 years ago

@richiethomas ack, one last thing, can you replace your commit subject with the PR subject so that the commit log shows what you did? I'll merge as soon as that's updated. I don't want to overwrite your commit as mine.

@geoffharcourt yep, I misunderstood your earlier comment about the PR description. Fixed now.

richiethomas commented 4 years ago

I'm still seeing the same issue, even when pointing the executable to my local. @geoffharcourt mind holding off on merging until I figure out what's up? Happy to close this for now if that's better.

richiethomas commented 4 years ago

Thanks @geoffharcourt, feel free to merge at your leisure. The problem was a functionality issue, not a CI issue. Specifically, I was previously using an || operator between the check for the parallelize flag and the check for the Ruby version. That will cause pg_restore to use multiple cores even if we haven't passed the flag, as long as the user's Ruby version is high enough. I replaced that with an && operator and it looks to be working fine now. Added another test to cover this case, as well.

geoffharcourt commented 4 years ago

I saw that and thought it was intentional! Nice fix.

geoffharcourt commented 4 years ago

This will require a major release of Parity, which I'll queue up for the next week.