0012082: deactivate failing scheduled imports

* adds failcount (int) and lastfail (error message) cols
* no longer run imports for jobs with failcount = 5

https://forge.tine20.org/view.php?id=12082

Change-Id: I78495ebed4ab0084f7d92586d319dc9ac8f2f962
Reviewed-on: http://gerrit.tine20.com/customers/3423
Tested-by: Jenkins CI (http://ci.tine20.com/)
Reviewed-by: Philipp Schüle <p.schuele@metaways.de>

diff --git a/tests/tine20/Tinebase/ScheduledImportTest.php b/tests/tine20/Tinebase/ScheduledImportTest.php
index 97e985a..7b00615 100644 (file)
--- a/tests/tine20/Tinebase/ScheduledImportTest.php
+++ b/tests/tine20/Tinebase/ScheduledImportTest.php
@@ -117,18 +117,31 @@ class Tinebase_ScheduledImportTest extends TestCase
         $this->_uit->runNextScheduledImport();
         $all = $cc->search($filter);
         $this->assertEquals($seq, $all->getFirstRecord()->seq);
-        
-        // setting manual timestamp to force run again
-        $record->timestamp = $record->timestamp->subHour(1)->subSecond(1);
-        
-        $this->_uit->update($record);
-        
+
+        $this->_runAgain($record);
+
         $this->_uit->runNextScheduledImport();
         $all = $cc->search($filter);
         $this->assertEquals(7, $all->count());
     }
 
     /**
+     * make import run again
+     *
+     * @param Tinebase_Model_Import|array $record
+     */
+    protected function _runAgain($record)
+    {
+        if (! $record instanceof Tinebase_Model_Import) {
+            $record = new Tinebase_Model_Import($record);
+        }
+
+        // setting manual timestamp to force run again
+        $record->timestamp = $record->timestamp->subHour(1)->subSecond(1);
+        $this->_uit->update($record);
+    }
+
+    /**
      * @see 0011342: ics-scheduled import only imports 1 remote calendar
      */
     public function testMultipleScheduledImports()
@@ -161,4 +174,25 @@ class Tinebase_ScheduledImportTest extends TestCase
 
         $this->assertEquals(0, count($result), 'no imports should be found: ' . print_r($result->toArray(), true));
     }
+
+    /**
+     * @see 0012082: deactivate failing scheduled imports
+     */
+    public function testDeactivatingImport()
+    {
+        // create invalid import
+        $import1 = $this->createScheduledImport();
+
+        // run 5 (maxfailcount) times
+        for ($i = 1; $i <= Tinebase_Controller_ScheduledImport::MAXFAILCOUNT; $i++) {
+            $importRun = $this->_uit->runNextScheduledImport();
+            $this->assertTrue(isset($importRun['failcount']), 'failcount should exist (import run ' . $i . ')');
+            $this->assertEquals($i, $importRun['failcount'], 'failcount should increase: ' . print_r($importRun, true));
+            $this->_runAgain($importRun);
+        }
+
+        // check if import is deactivated
+        $importRun = $this->_uit->runNextScheduledImport();
+        $this->assertTrue($importRun === null, 'import should not run: ' . print_r($importRun, true));
+    }
 }

diff --git a/tine20/Tinebase/Controller/ScheduledImport.php b/tine20/Tinebase/Controller/ScheduledImport.php
index 09e5b82..b4862f9 100644 (file)
--- a/tine20/Tinebase/Controller/ScheduledImport.php
+++ b/tine20/Tinebase/Controller/ScheduledImport.php
@@ -23,6 +23,8 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
      * @var Tinebase_Controller_ScheduledImport
      */
     private static $instance = null;
+
+    const MAXFAILCOUNT = 5;
     
     /**
      * the constructor
@@ -68,9 +70,12 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
             return $this->_doScheduledImport($record)->toArray();
         }
         
-        return NULL;
+        return null;
     }
 
+    /**
+     * @return Tinebase_Model_ImportFilter
+     */
     public function getScheduledImportFilter()
     {
         $timestampBefore = null;
@@ -87,6 +92,9 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
         $aWeekAgo->subWeek(1);
 
         $filter = new Tinebase_Model_ImportFilter(array(array(
+                array('field' => 'failcount', 'operator' => 'greater', 'value' => 5),
+            ),
+            array(
             'condition' => 'OR', 'filters' => array(
                 array('field' => 'timestamp', 'operator' => 'isnull', 'value' => null),
                 array('condition' => 'AND', 'filters' => array(
@@ -116,7 +124,7 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
      * 
      * @param interval
      * @param recursive
-     * @return Object
+     * @return Tinebase_Model_Import|null
      */
     protected function _getNextScheduledImport()
     {
@@ -124,21 +132,30 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
 
         // Always sort by timestamp to ensure first in first out
         $pagination = new Tinebase_Model_Pagination(array(
-            'limit'     => 1,
+            'limit'     => 50,
             'sort'      => 'timestamp',
             'dir'       => 'ASC'
         ));
 
-        $record = $this->search($filter, $pagination)->getFirstRecord();
-        
-        if (! $record) {
-            if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) {
-                Tinebase_Core::getLogger()->debug(__METHOD__ . ' ' . __LINE__ . ' No ScheduledImport could be found.');
-            }
+        $records = $this->search($filter, $pagination);
 
-            return NULL;
+        foreach ($records as $record) {
+            // TODO add failcount to filter in getScheduledImportFilter as
+            //   no more valid imports are run if we have 50+ failing imports
+            if ($record->failcount < self::MAXFAILCOUNT) {
+                return $record;
+            } else {
+                if (Tinebase_Core::isLogLevel(Zend_Log::INFO)) {
+                    Tinebase_Core::getLogger()->info(__METHOD__ . ' ' . __LINE__ . ' Too many failures, skipping import');
+                }
+            }
         }
-        return $record;
+        
+        if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) {
+            Tinebase_Core::getLogger()->debug(__METHOD__ . ' ' . __LINE__ . ' No valid ScheduledImport could be found.');
+        }
+
+        return null;
     }
     
     /**
@@ -197,13 +214,16 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
             try {
                 $importer = new $importer($options);
                 $importer->import($toImport);
+                $record->failcount = 0;
             } catch (Exception $e) {
                 if (Tinebase_Core::isLogLevel(Zend_Log::NOTICE)) {
                     Tinebase_Core::getLogger()->notice(__METHOD__ . ' ' . __LINE__
                         . ' Import failed.');
                 }
-                // TODO log failure message in import record
                 Tinebase_Exception::log($e);
+
+                $record->lastfail = $e->getMessage();
+                $record->failcount = $record->failcount + 1;
             }
 
             if ($record->interval === Tinebase_Model_Import::INTERVAL_ONCE || !$record->timestamp instanceof Tinebase_DateTime) {
@@ -222,7 +242,6 @@ class Tinebase_Controller_ScheduledImport extends Tinebase_Controller_Record_Abs
                     break;
             }
 
-            // update record
             $record = $this->update($record);
             
         } else {

diff --git a/tine20/Tinebase/Model/Import.php b/tine20/Tinebase/Model/Import.php
index f64c2db..1346691 100644 (file)
--- a/tine20/Tinebase/Model/Import.php
+++ b/tine20/Tinebase/Model/Import.php
@@ -91,6 +91,8 @@ class Tinebase_Model_Import extends Tinebase_Record_Abstract
         'sourcetype'         => array('presence' => 'required'),
         'options'            => array('allowEmpty' => true),
         'source'             => array('allowEmpty' => true),
+        'failcount'          => array('allowEmpty' => true, 'default' => 0),
+        'lastfail'           => array('allowEmpty' => true),
         'created_by'         => array('allowEmpty' => true),
         'creation_time'      => array('allowEmpty' => true),
         'last_modified_by'   => array('allowEmpty' => true),

diff --git a/tine20/Tinebase/Setup/Update/Release9.php b/tine20/Tinebase/Setup/Update/Release9.php
index ba8d2e5..8a3ff81 100644 (file)
--- a/tine20/Tinebase/Setup/Update/Release9.php
+++ b/tine20/Tinebase/Setup/Update/Release9.php
@@ -12,7 +12,7 @@ class Tinebase_Setup_Update_Release9 extends Setup_Update_Abstract
 {
     /**
      * update to 9.1
-     * 
+     *
      * @see 0011178: allow to lock preferences for individual users
      */
     public function update_0()
@@ -97,7 +97,8 @@ class Tinebase_Setup_Update_Release9 extends Setup_Update_Abstract
         // delete index unique-fields
         try {
             $this->_backend->dropIndex('relations', 'unique-fields');
-        } catch (Exception $e) {}
+        } catch (Exception $e) {
+        }
         $declaration = new Setup_Backend_Schema_Index_Xml('
             <index>
                 <name>unique-fields</name>
@@ -203,4 +204,31 @@ class Tinebase_Setup_Update_Release9 extends Setup_Update_Abstract
         $this->createTable('path', $declaration);
         $this->setApplicationVersion('Tinebase', '9.7');
     }
+
+    /**
+     * update to 9.8
+     *
+     * adds failcount+lastfail to scheduled imports
+     *
+     * @see 0012082: deactivate failing scheduled imports
+     */
+    public function update_7()
+    {
+        if ($this->getTableVersion('import') < 2) {
+            $declaration = new Setup_Backend_Schema_Field_Xml('<field>
+                    <name>failcount</name>
+                    <type>integer</type>
+                </field>');
+            $this->_backend->addCol('import', $declaration);
+
+            $declaration = new Setup_Backend_Schema_Field_Xml('<field>
+                    <name>lastfail</name>
+                    <type>text</type>
+                </field>');
+            $this->_backend->addCol('import', $declaration);
+        }
+
+        $this->setTableVersion('import', '2');
+        $this->setApplicationVersion('Tinebase', '9.8');
+    }
 }

diff --git a/tine20/Tinebase/Setup/setup.xml b/tine20/Tinebase/Setup/setup.xml
index a5d111c..544f74f 100644 (file)
--- a/tine20/Tinebase/Setup/setup.xml
+++ b/tine20/Tinebase/Setup/setup.xml
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <application>
     <name>Tinebase</name>
-    <version>9.7</version>
+    <version>9.8</version>
     <tables>
         <table>
             <name>applications</name>
@@ -2736,7 +2736,7 @@
         </table>
         <table>
             <name>import</name>
-            <version>1</version>
+            <version>2</version>
             <declaration>
                 <field>
                     <name>id</name>
@@ -2793,6 +2793,14 @@
                     <name>options</name>
                     <type>text</type>
                 </field>
+                <field>
+                    <name>failcount</name>
+                    <type>integer</type>
+                </field>
+                <field>
+                    <name>lastfail</name>
+                    <type>text</type>
+                </field>
                 <index>
                     <name>id</name>
                     <primary>true</primary>