Refactor responsibilities for LockWritesManager / lock_writes rake (Follow-up from "Include detached partition tables in truncate_legacy_tables")
The main ask is to consolidate most logic from the rake task to the LockWritesManager
. As a consequence we can then reduce duplications in specs between spec/tasks/gitlab/db/lock_writes_rake_spec.rb
and spec/lib/gitlab/database/lock_writes_manager_spec.rb
.
The following discussions from !103703 (merged) should be addressed:
-
@OmarQunsulGitlab started a discussion: (non-blocking nitpick), maybe it can be moved to the
initialize
, so thatextract_schema_qualified_name
is called only once. -
@OmarQunsulGitlab started a discussion: What do you think about adding another group of tests (context) for locking partitions as well, since they are not covered here yet, but the functionality has been added It can be minimal. Doesn't have to cover as much as we do for regular table
'_test_table'
now. I am fine with follow up for it.Update: I saw that we cover partitions locking in the rake task specs. Which might make this comment less relevant :)
-
@OmarQunsulGitlab started a discussion: (+1 comment) @tkuah there is no rspec for this one, right?
-
@DylanGriffith started a discussion: Thought (non-blocking): I wonder if this should be added to
ActiveRecord::Base
? It feels a little out of place here. -
@DylanGriffith started a discussion: Thought (non-blocking): Now that it seems all the responsibility here is living in
LockWritesManager
it doesn't make much sense that the looping is happening outside of this class. It might look tidier just moving it all intoLockWritesManager
with the downside of that class having a little more responsibility. It might also simplify things if we're not passing data back and forth via yield and method arguments but that might just be a personal preference to useyield
less... -
@DylanGriffith started a discussion: (+1 comment) @tkuah this MR seems fine to me but I left some thoughts you may wish to address or not. I also think Omar's suggestions around testing seem like they should be looked into.
I must admit I'm not really familiar with how detached partitions work and whether or not this will capture edge cases that we've been seeing. I guess you've already figured this out in #382130 (closed) . I remember in production we did also have the edge cases with the initial partitions that were created in a migration where we converted a table to a partition of itself. I'm not clear if this solution is designed to help with that or not.