Arvados

rebased

19709-boot-db-migrate @ 41b9b83812826b77034fe13ea34047e194b1027f -- developer-run-tests: #3408

Tom Clegg wrote in #note-8:

19709-boot-db-migrate @ 41b9b83812826b77034fe13ea34047e194b1027f -- developer-run-tests: #3408

I don't have any big concerns about this code as-is, I mostly have questions about why this approach was chosen.

What are the reason(s) we cannot or should not just call db:migrate directly every time? It seems to handle the case of "no migrations need to be run" just fine. I see the comment about rake commands being "[relatively slow]" which, sure, I'll grant that, but is that the only reason?

If we can't run db:migrate every time, Rails provides a db:migrate:status task to report what migrations have and haven't been done. I understand this gets us back into slowness, but I feel like it's a pretty big upside to avoid reimplementing Rails' own logic for this, and potentially having subtle discrepancies. Are there other reasons besides slowness you decided against using this?

If we must do it ourselves, is it worth verifying that the migration version we pull out of the filename (d.Name()[:cut]) is numeric before we add it to the todo list? Rails itself seems to require this, so our implementation could catch stray files lying in that directory that Rails itself ignores. It shouldn't be too bad if it happens, just a noop db:migrate run, but if we're that concerned about avoiding those maybe it's worth it?

I think it would also be nice to see some of this "why this approach" discussion in the commit message, whatever we end up doing, in case future readers have questions.

Thanks.

Yes, this is really just about timing. I've added this to make it less mysterious:

       // In principle, we could use "rake db:migrate:status" or skip
       // this check entirely and let "rake db:migrate" be a no-op
       // most of the time.  However, in the most common case when
       // there are no new migrations, that would add ~2s to startup
       // time / downtime during service restart.

I've added the only-digits-before-underscore check.

Going out of sync with Rails logic is a fair point. I think I can address that with a test case.

Test that schema_migrations and db/migrate/*.rb line up the way we expect.

19709-boot-db-migrate @ 228028e523983a847239fd4a5530d3e54b27cfa5 -- developer-run-tests: #3414

Tom Clegg wrote in #note-10:

Yes, this is really just about timing. I've added this to make it less mysterious:

Okay, thanks. I'll just note, if it were me, I would've taken the other trade. Two seconds of downtime for an event that should be relatively rare seems okay to save us all this code. But I don't think it's my decision, especially not during the review.

Thanks for the test, that is nice.

My only concern with the new version is that it now fails if there are non-migration files in the db/migrate directory. This seems needlessly strict to me and admin-unfriendly to stop the boot because of these extra files. (I'm guessing the extra files that are most likely to end up there is backup files, or migrations intentionally moved out of the way, when the admin is in a bad spot and trying to un-wedge things. They might just break things more, but so could modifying the migrations in-place, and we're not defending against that.) I think it would be nicer to log each file being skipped but otherwise continue as normal. What do you think?

Good point. Updated to warn (instead of erroring out) on unexpected files in db/migrate/.

19709-boot-db-migrate @ da416e33b7e9fb51c27662ef111bdc4fafcadbcf -- developer-run-tests: #3415

Tom Clegg wrote in #note-13:

Good point. Updated to warn (instead of erroring out) on unexpected files in db/migrate/.

19709-boot-db-migrate @ da416e33b7e9fb51c27662ef111bdc4fafcadbcf -- developer-run-tests: #3415

Thanks, LGTM.