@gravitystorm requested changes on this pull request.
In general I'm neutral on this idea. I've read the operations task and I
haven't seen convincing evidence that this is the best way to solve the
operational problems. From a codebase point of view, this adds significant
complexity to all deployments, so we need to make really sure that it's worth
doing.
For this PR, in addition to my inline notes, I would say:
* There's no task included to migrate the data from the old database to the new
one. This is critical, particularly for non-OSMF deployments that you are not
personally managing.
* There's no migration strategy. Do we expect all the traces to disappear from
the site, then slowly reappear as the migration progresses? Or is there going
to be a (several-day?) outage during the migration?
* Or should the general strategy be more like the ["renaming a table"
strategy](https://github.com/ankane/strong_migrations#renaming-a-table)
documented in strong_migrations?
* When this PR runs, the site will instantly use the new database for new
uploads. This will then immediately conflict with any data that is being
migrated from the old database to the new (e.g. primary key reuse).
> @@ -12,6 +12,10 @@ class ApplicationController < ActionController::Base
rescue_from RailsParam::InvalidParameterError, :with => :invalid_parameter
+ rescue_from ActiveRecord::ConnectionNotEstablished,
+ ActiveRecord::DatabaseConnectionError,
+ ActiveRecord::NoDatabaseError, :with => :gps_database_unavailable
+
This isn't correct, those errors could be for other reasons
> @@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class GpsRecord < ApplicationRecord
We should consider the naming of this abstract class (and the database name)
more carefully.
When OSM was created, "GPS" and satellite positioning was more or less
interchangeable. However, time has moved on, and the American GPS system is
only one of 4 different GNSS systems.
GNSS is, however, another acronym, and not very self-explanatory - nor is it in
common use.
Was the decision to call the database "gps" and the model "GpsRecord"
deliberate? Or would something involving "trace" be more consistent with e.g.
the model naming?
> @@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+# Drop the GPS tables (gps_points, gpx_file_tags, gpx_files) from the main
database.
+# These tables now live in a separate GPS database (see GpsRecord model).
+class DropGpsTablesFromMainDb < ActiveRecord::Migration[8.1]
If anyone runs this migration, they will lose all of their traces in their
development database.
It's also something that we couldn't merge as-is, since anyone deploying this
PR would lose all the trace data from their production database.
> @@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+class CreateGpsTables < ActiveRecord::Migration[8.1]
+ def change
+ create_enum :gpx_visibility_enum, %w[private public trackable identifiable]
+
+ create_table :gpx_files, :id => :bigint do |t|
If we're going to create a new database, then I think this is an opportunity to
fix the table naming, to match the model names.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7111#pullrequestreview-4371472265
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/7111/review/[email protected]>
_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
