[ARVADOS] updated: 3c69428a8165f485140f1bbbbb899a308e289df2

[Git user]

Summary of changes:
 services/api/app/models/container.rb             | 21 ++++++++++-----------
 services/api/test/unit/container_request_test.rb |  1 +
 services/api/test/unit/container_test.rb         | 22 +++++++++++-----------
 3 files changed, 22 insertions(+), 22 deletions(-)

       via  3c69428a8165f485140f1bbbbb899a308e289df2 (commit)
      from  a02b4f41cb495ecfea49ac0da1f6da0aec385f7d (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 3c69428a8165f485140f1bbbbb899a308e289df2
Author: Lucas Di Pentima <lucas at curoverse.com>
Date:   Wed Sep 14 12:43:55 2016 -0300

    9623: Several fixes addressing review comments:
    * Fixed ordering for completed containers selection
    * Search for Locked or Queued containers in just one query
    * Fixed several test issues

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 1784ae5..c1c3eae 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -90,19 +90,18 @@ class Container < ArvadosModel
     # Check for Completed candidates that only had consistent outputs.
     completed = candidates.where(state: Complete).where(exit_code: 0)
     if completed.select("output").group('output').limit(2).length == 1
-      return completed.order('finished_at desc').limit(1).first
+      return completed.order('finished_at asc').limit(1).first
     end
 
-    # First, check for Running candidates and return the most likely to finish sooner,
-    # then for Locked or Queued ones and return the most likely to start first.
-    [
-      [Running, 'progress desc, started_at asc'],
-      [Locked, 'priority desc, created_at asc'],
-      [Queued, 'priority desc, created_at asc']
-    ].each do |candidate_state, ordering_criteria|
-      selected = candidates.where(state: candidate_state).order(ordering_criteria).limit(1).first
-      return selected if not selected.nil?
-    end
+    # Check for Running candidates and return the most likely to finish sooner.
+    running = candidates.where(state: Running).
+      order('progress desc, started_at asc').limit(1).first
+    return running if not running.nil?
+
+    # Check for Locked or Queued ones and return the most likely to start first.
+    locked_or_queued = candidates.where("state IN (?)", [Locked, Queued]).
+      order('state asc, priority desc, created_at asc').limit(1).first
+    return locked_or_queued if not locked_or_queued.nil?
 
     # No suitable candidate found.
     nil
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 7083b35..d75472c 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -413,6 +413,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
                                                     environment: env1}))
       cr2 = create_minimal_req!(common_attrs.merge({state: ContainerRequest::Uncommitted,
                                                     environment: env2}))
+      assert_not_nil cr1.container_uuid
       assert_nil cr2.container_uuid
 
       # Update cr2 to commited state and check for container equality on both cases,
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 6501f21..85741bb 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -139,7 +139,7 @@ class ContainerTest < ActiveSupport::TestCase
 
     reused = Container.find_reusable(common_attrs)
     assert_not_nil reused
-    assert_equal reused.uuid, c_recent.uuid
+    assert_equal reused.uuid, c_older.uuid
   end
 
   test "find_reusable method should not select completed container when inconsistent outputs exist" do
@@ -151,17 +151,17 @@ class ContainerTest < ActiveSupport::TestCase
       log: 'test',
     }
 
-    c_older, _ = minimal_new(common_attrs)
-    c_recent, _ = minimal_new(common_attrs)
+    c_output1, _ = minimal_new(common_attrs)
+    c_output2, _ = minimal_new(common_attrs)
 
     set_user_from_auth :dispatch1
-    c_older.update_attributes!({state: Container::Locked})
-    c_older.update_attributes!({state: Container::Running})
-    c_older.update_attributes!(completed_attrs.merge({output: 'output 1'}))
+    c_output1.update_attributes!({state: Container::Locked})
+    c_output1.update_attributes!({state: Container::Running})
+    c_output1.update_attributes!(completed_attrs.merge({output: 'output 1'}))
 
-    c_recent.update_attributes!({state: Container::Locked})
-    c_recent.update_attributes!({state: Container::Running})
-    c_recent.update_attributes!(completed_attrs.merge({output: 'output 2'}))
+    c_output2.update_attributes!({state: Container::Locked})
+    c_output2.update_attributes!({state: Container::Running})
+    c_output2.update_attributes!(completed_attrs.merge({output: 'output 2'}))
 
     reused = Container.find_reusable(common_attrs)
     assert_nil reused
@@ -185,7 +185,7 @@ class ContainerTest < ActiveSupport::TestCase
                                                 progress: 0.15})
     reused = Container.find_reusable(common_attrs)
     assert_not_nil reused
-    # Winner is the one that started first
+    # Selected container is the one that started first
     assert_equal reused.uuid, c_faster_started_first.uuid
   end
 
@@ -207,7 +207,7 @@ class ContainerTest < ActiveSupport::TestCase
                                                 progress: 0.2})
     reused = Container.find_reusable(common_attrs)
     assert_not_nil reused
-    # Winner is the one with most progress done
+    # Selected container is the one with most progress done
     assert_equal reused.uuid, c_faster_started_second.uuid
   end
 

-----------------------------------------------------------------------


hooks/post-receive
--