From 3f3de0a234b5dba67d9d699817677ae5afeacb83 Mon Sep 17 00:00:00 2001 From: christopherganda Date: Thu, 2 Jan 2025 22:35:19 +0700 Subject: [PATCH 1/4] set reminder on readme for deadlock --- system-design/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/system-design/README.md b/system-design/README.md index ece3760..1c27ae7 100644 --- a/system-design/README.md +++ b/system-design/README.md @@ -23,6 +23,7 @@ This document contains the details of ideas and implementation for time tracker 3. For any clock-ins, every second clock-in will be considered a clock out for the last clock-in for us to calculate sleep record. 4. Since the feature is quite simple, most part is enough with simple error message, but it is better to implement proper logging. 5. Need proper error handling because ActiveModel::Error's full_messages return message with key. +6. Need to implement retry mechanism for concurrent requests to mitigate deadlocks on locking. ## Non-Functional Requirements 1. **Scalability** From 9c090ca9d9400725bc8e6e59381276d2d454a211 Mon Sep 17 00:00:00 2001 From: christopherganda Date: Fri, 3 Jan 2025 08:44:41 +0700 Subject: [PATCH 2/4] fix ordering filter to improve performance and add composite index for clock ins --- ..._index_to_user_follows_on_follower_and_followee.rb | 2 +- ...d_index_clock_ins_on_is_clocked_out_and_user_id.rb | 11 +++++++++++ db/schema.rb | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20250103014113_add_index_clock_ins_on_is_clocked_out_and_user_id.rb diff --git a/db/migrate/20250102060751_add_index_to_user_follows_on_follower_and_followee.rb b/db/migrate/20250102060751_add_index_to_user_follows_on_follower_and_followee.rb index f415519..36b627f 100644 --- a/db/migrate/20250102060751_add_index_to_user_follows_on_follower_and_followee.rb +++ b/db/migrate/20250102060751_add_index_to_user_follows_on_follower_and_followee.rb @@ -4,6 +4,6 @@ def up end def down - add_index :user_follows, name: 'index_user_follows_on_follower_id_and_followee_id' + remove_index :user_follows, name: 'index_user_follows_on_follower_id_and_followee_id' end end diff --git a/db/migrate/20250103014113_add_index_clock_ins_on_is_clocked_out_and_user_id.rb b/db/migrate/20250103014113_add_index_clock_ins_on_is_clocked_out_and_user_id.rb new file mode 100644 index 0000000..be65c23 --- /dev/null +++ b/db/migrate/20250103014113_add_index_clock_ins_on_is_clocked_out_and_user_id.rb @@ -0,0 +1,11 @@ +class AddIndexClockInsOnIsClockedOutAndUserId < ActiveRecord::Migration[7.0] + def up + add_index :clock_ins, [:is_clocked_out, :user_id], name: 'index_clock_ins_on_is_clocked_out_and_user_id' + remove_index :clock_ins, [:user_id, :clocked_in_at] + end + + def down + remove_index :user_follows, name: 'index_clock_ins_on_is_clocked_out_and_user_id' + add_index :clock_ins, [:user_id, :clocked_in_at] + end +end diff --git a/db/schema.rb b/db/schema.rb index bfe3597..7cfcca2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2025_01_02_060751) do +ActiveRecord::Schema[7.0].define(version: 2025_01_03_014113) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -18,7 +18,7 @@ t.bigint "user_id", null: false t.datetime "clocked_in_at", null: false t.boolean "is_clocked_out", default: false, null: false - t.index ["user_id", "clocked_in_at"], name: "index_clock_ins_on_user_id_and_clocked_in_at" + t.index ["is_clocked_out", "user_id"], name: "index_clock_ins_on_is_clocked_out_and_user_id" t.index ["user_id"], name: "index_clock_ins_on_user_id" end From 1ace4cf72039a6c6f0bdf83898f927c7d22e1af0 Mon Sep 17 00:00:00 2001 From: christopherganda Date: Fri, 3 Jan 2025 09:05:34 +0700 Subject: [PATCH 3/4] add module for globalize retry mechanism --- app/controllers/concerns/retryable.rb | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 app/controllers/concerns/retryable.rb diff --git a/app/controllers/concerns/retryable.rb b/app/controllers/concerns/retryable.rb new file mode 100644 index 0000000..b738539 --- /dev/null +++ b/app/controllers/concerns/retryable.rb @@ -0,0 +1,28 @@ +module Retryable + MAX_RETRIES = 3 + RETRY_DELAY = 0.5 # seconds + + def with_retry(max_retries: MAX_RETRIES, delay: RETRY_DELAY) + attempts = 0 + + begin + yield + rescue ActiveRecord::Deadlocked => e + attempts += 1 + Rails.logger.warn("Retryable Error: #{e.class} - #{e.message}. Attempt #{attempts} of #{max_retries}.") + if attempts < max_retries + sleep(delay * attempts) + retry + else + Rails.logger.error("Retries exhausted for #{e.class}: #{e.message}") + raise e + end + rescue ActiveRecord::ActiveRecordError => e + Rails.logger.error("ActiveRecord Error: #{e.class} - #{e.message}") + render_error(:unprocessable_entity, e.message) + rescue StandardError => e + Rails.logger.error("Unexpected Error: #{e.class} - #{e.message}") + render_error(:internal_server_error, e.message) + end + end +end \ No newline at end of file From f0831c2518a3a6cfacef3907ded71151e9b3bbd9 Mon Sep 17 00:00:00 2001 From: christopherganda Date: Fri, 3 Jan 2025 09:05:56 +0700 Subject: [PATCH 4/4] add retry mechanism on clock ins to minimize deadlock --- app/controllers/clock_ins_controller.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/clock_ins_controller.rb b/app/controllers/clock_ins_controller.rb index 448ffc5..dbcb8b6 100644 --- a/app/controllers/clock_ins_controller.rb +++ b/app/controllers/clock_ins_controller.rb @@ -1,14 +1,16 @@ class ClockInsController < ApplicationController include UserFinder + include Retryable def upsert current_time = Time.now - begin + with_retry do ActiveRecord::Base.transaction do last_unused_clock_in = ClockIn.where( - user_id: @user.id, - is_clocked_out: false) + is_clocked_out: false, + user_id: @user.id + ) .order(clocked_in_at: :desc) .limit(1) .lock(true) @@ -27,14 +29,12 @@ def upsert else ClockIn.create!( user_id: @user.id, - clocked_in_at: Time.now + clocked_in_at: current_time ) render_success(@user.clock_ins_json, I18n.t('success.messages.clock_in_success')) end end - rescue ActiveRecord::ActiveRecordError => e - render_error(:unprocessable_entity, e.message) end end