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

[Hugh O. Brock]

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