pJeyakumar / noticed_upgrade_project

2 stars 0 forks source link

Create upgrade guide/strategy based on this project's experiences #4

Closed srb- closed 3 months ago

srb- commented 3 months ago

Will garner inspiration from the Noticed Update guide This issue #6 should also be worked on in tandem.


Game Plan

Option 1: ✅

  1. Upgrade Noticed gem to 2.3.2.
  2. Create the new noticed tables (empty).
  3. Make the necessary code changes so that the "new notifications" work.
    • any new notifications that users create will be created in the "new notifications" table
  4. Run the job to populate the "new notifications" table using the "old notifications" table (i.e.
    • the job will convert the old notifications in descending order by date.
  5. Remove the job.
  6. Drop the old notifications table.

Pros

Cons

Suggestion

Option 2: ❌

  1. Create the new noticed tables (empty).
  2. Create mock models, helpers, and methods that are necessary to run the job.
  3. Run the job to populate these tables with the "new-style" of notifications.
  4. Remove the mock models, helpers, and methods that were created.
  5. Upgrade Noticed gem to 2.3.2 and make the necessary code changes so that the new notifications work.
  6. Run the job again to convert any old notifications that have been recently created.
  7. Remove the job.
  8. Drop the old notifications table.

Pros

Cons


Changes to be made:

  1. Create the following migration files via rails g migration CreateNoticedTables and rails g migration AddNotificationsCountToNoticedEvent and copy the contents below into those corresponding files.

    class CreateNoticedTables < ActiveRecord::Migration[7.1]
    def change
    primary_key_type, foreign_key_type = primary_and_foreign_key_types
    create_table :noticed_events, id: primary_key_type do |t|
      t.string :type
      t.belongs_to :record, polymorphic: true, type: foreign_key_type
      if t.respond_to?(:jsonb)
        t.jsonb :params
      else
        t.json :params
      end
    
      t.timestamps
    end
    
    create_table :noticed_notifications, id: primary_key_type do |t|
      t.string :type
      t.belongs_to :event, null: false, type: foreign_key_type
      t.belongs_to :recipient, polymorphic: true, null: false, type: foreign_key_type
      t.datetime :read_at
      t.datetime :seen_at
    
      t.timestamps
    end
    end
    
    private
    
    def primary_and_foreign_key_types
    config = Rails.configuration.generators
    setting = config.options[config.orm][:primary_key_type]
    primary_key_type = setting || :primary_key
    foreign_key_type = setting || :bigint
    [primary_key_type, foreign_key_type]
    end
    end
class AddNotificationsCountToNoticedEvent < ActiveRecord::Migration[7.1]
  def change
    add_column :noticed_events, :notifications_count, :integer
  end
end
  1. Run rails db:migrate to add the above tables to the database.
  2. Update the noticed gem in the Gemfile to gem "noticed", "~> 2.3", ">= 2.3.2" and run bundle install
  3. Make the following changes to the codebase:
    • Delete the Notification model in app/models/notification.rb.
    • Noticed v2 adds models managed by the gem.
    • Rename the app/notifications folder to app/notifiers
    • Rename all notification classes to be notifier by running the following code in rails console
      
      require 'fileutils'

dir = 'app/notifiers'

Dir.glob("#{dir}/*_notification.rb").each do |file| new_file = file.gsub('notification', 'notifier') FileUtils.mv(file, new_file) puts "Renamed: #{file} to #{new_file}" end

- Rename the `spec/notifications` folder to `spec/notifiers`
- Rename all `notification` classes to be `notifier` by running the following code in `rails console`
```ruby
require 'fileutils'

dir = 'spec/notifiers'

Dir.glob("#{dir}/*_notification_spec.rb").each do |file|
new_file = file.gsub('notification', 'notifier')
  FileUtils.mv(file, new_file)
  puts "Renamed: #{file} to #{new_file}"
end
  1. To migrate the data into new tables:

    • Create a job to loop through all existing notifications and create a new record for each one.
    • We can implement an ActiveJob (in app/jobs/migrate_notifications_job.rb) like this one:
      
      class MigrateNotificationsJob < ApplicationJob
      queue_as :default

    ADJUST BATCH_SIZE AND LIMIT AS NEEDED

    BATCH_SIZE = 10 LIMIT = 100

    Define the Notification model to access the old table

    class Notification < ApplicationRecord self.inheritance_column = nil end

    def perform(*args)
    total_processed = 0

    last_processed_at = get_last_processed_at

    Process notifications in batches

    Notifications are ordered by created_at date (newest first)

    We then scope them to only included notifications OLDER than our last_processed_at notification

    Then we run them in batches, in order of PRIMARY KEY descending (should be the same as created_at desc), find_in_batches MUST be ordered by PRIMARY KEY (we can choose either ascending or descending order).

    Notification.order(created_at: :desc).where("created_at < ?", last_processed_at).limit(LIMIT).find_in_batches(order: :desc, batch_size: BATCH_SIZE) do |batch| batch.each do |notification| return if total_processed >= LIMIT migrate_notification(notification) total_processed += 1
    end
    end end

    private

    def get_last_processed_at last_event = Noticed::Event.order(created_at: :desc).last last_event ? last_event.created_at : DateTime.now end

    def migrate_notification(notification)

    Ensure the record has not already been migrated, Not sure about this query working in "Notice::Event", but something like this.

    return if Noticed::Event.where(created_at: notification.created_at, params: Noticed::Coder.load(notification.params).with_indifferent_access).exists?

    attributes = notification.attributes.slice("type", "created_at", "updated_at").with_indifferent_access attributes[:type] = attributes[:type].sub("Notification", "Notifier") attributes[:params] = Noticed::Coder.load(notification.params) attributes[:params] = {} if attributes[:params].try(:has_key?, "noticed_error") # Skip invalid records

    attributes[:notifications_attributes] = [{ type: "#{attributes[:type]}::Notification", recipient_type: notification.recipient_type, recipient_id: notification.recipient_id, read_at: notification.read_at, seen_at: notification.read_at, created_at: notification.created_at, updated_at: notification.updated_at }]

    Noticed::Event.create!(attributes) end end

    
    - After verifying that each "old notification" has a corresponding "new notification" and Noticed is working without error, **Drop** the "old notification" table
jonathanloos commented 3 months ago

Hey folks! So here is an example of a notification system and how it should be structured for this project.

Usage

# notifications
gem "noticed", "~> 1.6.0"

Directory structure

app
> notifications
  > new_aircraft_created_notification.rb
> mailers
  > aircraft_mailer.rb
> views
  > aircraft_mailer
    > created_email.html.erb

Invocation

NewAircraftCreatedNotification.with(aircraft: aircraft).deliver_later(NewAircraftCreatedNotification.targets)

Notification Class

class NewAircraftCreatedNotification < ApplicationNotification
  deliver_by :database, if: :database_notifications?
  deliver_by :email, mailer: "AircraftMailer", method: "created_email", if: :email_notifications?

  # Variables that tells the system if it should send a notification if the recipient isn't subscribed
  DEFAULT_SEND_NOTIFICATION = true
  DEFAULT_SEND_EMAIL_NOTIFICATION = true

  # Add required params
  param :aircraft

  def message
    "#{params[:aircraft].name} has been created for use in the flight sim."
  end

  def url
    aircraft_path(params[:aircraft])
  end

  def database_notifications?
    DEFAULT_SEND_NOTIFICATION
  end

  def email_notifications?
    DEFAULT_SEND_EMAIL_NOTIFICATION
  end

  def self.targets
    # example of only sending notifications to admins. Really this just has to be an ActiveRecord::Relation of (likely) User objects.
    User.admin
  end
end

Mailer

User Model

class User < ApplicationRecord
  has_many :notifications, as: :recipient, dependent: :destroy

  # Helper for associating and destroying Notification records where(params: {object: self}).
  has_noticed_notifications
  has_noticed_notifications param_name: :assigned_to, destroy: false, model_name: "Notification"
  has_noticed_notifications param_name: :requester, destroy: false, model_name: "Notification"
  has_noticed_notifications param_name: :granted_by, destroy: false, model_name: "Notification"
end

Email Templates

<%# views/aircrafts_mailer/created_email %>

<p>Hello <%= @user.full_name %>,</p>
<br/>

<p>Aircraft <%= @aircraft %> has been created in the flight simulator.</p>
<br/>

<%= link_to "Check it out here", aircraft_url(@aircraft) %>

Email testing

We use letter_opener to preview emails locally.

pJeyakumar commented 3 months ago

@jonathanloos are we required to make any snapshots of the DB before we run the migrations? How are our DB backups structured (on the app).

jonathanloos commented 3 months ago

Yup our app's db is backed up with snapshots so there's no need to worry about us doing them. However, a rollback strategy without the backups (if possible) would be good to consider.

srb- commented 3 months ago

Looks good - assuming we make sure we update this doc with any snags/workarounds we hit when actually do https://github.com/pJeyakumar/noticed_upgrade_project/issues/3

@jonathanloos Are you using the AWS Aurora snapshots (every 5 minutes?), out of curiosity? Or what's the frequency of backup? Also, when's the last time you tested restoring from a backup, just wondering?

@pJeyakumar will the rake migration task cause downtime? How long would we expect it to last - I guess staging/dev will allow us to test realistically?

How will we validate we haven't lost any notifications in the process, on prod? Making sure row counts match?

pJeyakumar commented 3 months ago

@srb- the rake migration shouldn't cause any downtime given that we do our work in 2 distinct tasks.

  1. Run the migration to create the new records for notifications (and we can also make sure that the type value of these notifications are changed from Notification to Notifier) in our first deployment.
    • We would want to ensure that the app can work with both the new notifications table and old notifications table.
  2. After confirming that the app works with the new notifications table, we will run another deployment to clean up the codebase with anything that is no longer needed and drop the old notifications table. Doing so should result in no downtime whilst we are changing the notifications in the DB.

To answer your second question:

srb- commented 3 months ago

@pJeyakumar awesome, no downtime. Is there any concern with a notification being created in the old table while the migration is going on, and it gets missed? (e.g. any table locking, or after hours deployment, needed?)

You mentioned the app will support both old and new Notifications at the same time. How will we toggle that? Feature flag (do we use those?) or through a new deployment?

jonathanloos commented 3 months ago

Looks good - assuming we make sure we update this doc with any snags/workarounds we hit when actually do #3

@jonathanloos Are you using the AWS Aurora snapshots (every 5 minutes?), out of curiosity? Or what's the frequency of backup? Also, when's the last time you tested restoring from a backup, just wondering?

@pJeyakumar will the rake migration task cause downtime? How long would we expect it to last - I guess staging/dev will allow us to test realistically?

How will we validate we haven't lost any notifications in the process, on prod? Making sure row counts match?

@srb- we haven't done a formal restoration in a long time, but the staging restoration is very similar to what we would do. I would need to connect with our AWS resource to check the procedure. As for the frequency I would also need to confirm that! (sorry not much answers).

hvillero commented 3 months ago

@jonathanloos , For this upgrade, I understand that we may want to avoid downtime. However, considering a potential rollback situation, is there a good time during the day when we could run the upgrade if downtime becomes necessary? Additionally, knowing how long it would take to restore the previous database will help us estimate the required window of time.

pJeyakumar commented 3 months ago

@pJeyakumar awesome, no downtime. Is there any concern with a notification being created in the old table while the migration is going on, and it gets missed? (e.g. any table locking, or after hours deployment, needed?)

You mentioned the app will support both old and new Notifications at the same time. How will we toggle that? Feature flag (do we use those?) or through a new deployment?

With migrations of this size, we typically have a button (in our Developer settings page, which is on the app) to run the rake file in batches.

As for how the app will support both old and new notifications at the same time, we do not have Feature flags on the app, meaning we'd do it through a new deployment (my thinking is that our codebase should be able to support both the old/new notifications, if there is a noticeable difference between how we would handle both variations).

jonathanloos commented 3 months ago

@pJeyakumar all looks good here, but it's important to note here that the tasks will not be run as a migration. Rather, it will execute as a job we trigger manually. This has a couple benefits:

  1. Should the migration fail (incorrect/timeout) the application will continue to run.
  2. We can re-test migrations in staging after restorations daily.
  3. We can chunk/batch the updates if needed.

For testing the migrations, remember we're dealing with over a million records. So for the rake task here we would want to have an idea of how long it will need to run for to pull & create all new records.