[Ovirt-devel] [PATCH] support optimistic locking with lock_version

[steve linabery]

On Thu, May 22, 2008 at 11:23 AM, Hugh O. Brock <hbrock at redhat.com> wrote:
> On Wed, May 21, 2008 at 08:23:07PM -0500, steve linabery wrote:
>> Hi Ovirt,
>>
>> Edited the migration files to add the :lock_version column, which will
>> allow us to use optimistic locking by default.
>>
>> We can override this with more restrictive locking when needed, but
>> this should help catch stuff that might slip through the cracks.
>>
>
> Thanks Steve. I have a couple of questions in-line below, please
> indulge my ignorance if you would...
>
>> From 65af40d990b6d881f927609da7b63cbcbabe9e57 Mon Sep 17 00:00:00 2001
>> From: Steve Linabery <slinabery at gmail.com>
>> Date: Wed, 21 May 2008 20:16:39 -0500
>> Subject: [PATCH] Add lock_version attribute to all objects to enable optimistic locking.
>>  Edit migration files to use the 't.type :fieldname' syntax.
>>
>>
>> Signed-off-by: Steve Linabery <slinabery at gmail.com>
>> ---
>>  wui/src/db/migrate/001_create_pools.rb           |    3 +-
>>  wui/src/db/migrate/002_create_hosts.rb           |   21 ++++++------
>>  wui/src/db/migrate/003_create_nics.rb            |   15 +++++----
>>  wui/src/db/migrate/004_create_storage_volumes.rb |   28 +++++++++--------
>>  wui/src/db/migrate/005_create_quotas.rb          |   15 +++++----
>>  wui/src/db/migrate/006_create_vms.rb             |   37 ++++++++++++----------
>>  wui/src/db/migrate/007_create_tasks.rb           |   30 ++++++++++--------
>>  wui/src/db/migrate/008_create_permissions.rb     |    7 ++--
>>  8 files changed, 85 insertions(+), 71 deletions(-)
>>
>> diff --git a/wui/src/db/migrate/001_create_pools.rb b/wui/src/db/migrate/001_create_pools.rb
>> index 2348013..34c01d3 100644
>> --- a/wui/src/db/migrate/001_create_pools.rb
>> +++ b/wui/src/db/migrate/001_create_pools.rb
>> @@ -1,4 +1,4 @@
>> -#
>> +#
>>  # Copyright (C) 2008 Red Hat, Inc.
>>  # Written by Scott Seago <sseago at redhat.com>
>>  #
>> @@ -25,6 +25,7 @@ class CreatePools < ActiveRecord::Migration
>>        t.integer :parent_id
>>        t.integer :lft
>>        t.integer :rgt
>> +      t.integer :lock_version, :default => 0
>>        t.timestamps
>>      end
>>
>> diff --git a/wui/src/db/migrate/002_create_hosts.rb b/wui/src/db/migrate/002_create_hosts.rb
>> index 62d9edd..e8b26f2 100644
>> --- a/wui/src/db/migrate/002_create_hosts.rb
>> +++ b/wui/src/db/migrate/002_create_hosts.rb
>> @@ -1,4 +1,4 @@
>> -#
>> +#
>>  # Copyright (C) 2008 Red Hat, Inc.
>>  # Written by Scott Seago <sseago at redhat.com>
>>  #
>> @@ -20,15 +20,16 @@
>>  class CreateHosts < ActiveRecord::Migration
>>    def self.up
>>      create_table :hosts do |t|
>> -      t.column :uuid,                       :string
>> -      t.column :hypervisor_type,            :string
>> -      t.column :hostname,                   :string
>> -      t.column :num_cpus,                   :integer
>> -      t.column :cpu_speed,                  :integer
>> -      t.column :arch,                       :string
>> -      t.column :memory,                     :integer
>> -      t.column :is_disabled,                :integer
>> -      t.column :hardware_pool_id,           :integer, :null => false
>> +      t.string  :uuid
>> +      t.string  :hypervisor_type
>> +      t.string  :hostname
>> +      t.integer :num_cpus
>> +      t.integer :cpu_speed
>> +      t.string  :arch
>> +      t.integer :memory
>> +      t.integer :is_disabled
>> +      t.integer :hardware_pool_id, :null => false
>> +      t.integer :lock_version,     :default => 0
>>      end
>
> What's the reason for this change? Just tidying, or is there some
> advantage to using e.g. "t.string" rather than "t.column :foo
> :string"?
>
> [snip]
>
>> diff --git a/wui/src/db/migrate/008_create_permissions.rb b/wui/src/db/migrate/008_create_permissions.rb
>> index 0cc361c..69a0aa7 100644
>> --- a/wui/src/db/migrate/008_create_permissions.rb
>> +++ b/wui/src/db/migrate/008_create_permissions.rb
>> @@ -1,4 +1,4 @@
>> -#
>> +#
>>  # Copyright (C) 2008 Red Hat, Inc.
>>  # Written by Scott Seago <sseago at redhat.com>
>>  #
>> @@ -20,9 +20,10 @@
>>  class CreatePermissions < ActiveRecord::Migration
>>    def self.up
>>      create_table :permissions do |t|
>> -      t.string :user_role
>> -      t.string :user
>> +      t.string  :user_role
>> +      t.string  :user
>>        t.integer :pool_id
>> +      t.integer :lock_version, :default => 0
>>      end
>>    end
>>
>
> Other than that, looks good... I will ACK, but Scott can you take a
> look before I commit?
>
> Thanks,
> --Hugh
>

This is the latest-and-greatest Rails way of doing migrations. It's a
bit less cluttered, and it makes things like "t.timestamps" less
jarring, I guess. I think "t.timestamps" is new along with this way of
coding these files.

The newer migration files are in this style, and we decided to
"upgrade" the older files when/if they required edits (like now).

Thanks
Steve