Added retry system to get the results from HERE servers

services/geocoder/lib/hires_batch_geocoder.rb

@@ -12,6 +12,8 @@ module CartoDB
     DEFAULT_TIMEOUT = 5.hours
     POLLING_SLEEP_TIME = 5.seconds
     LOGGING_TIME = 5.minutes
+    DOWNLOAD_RETRIES = 5
+    DOWLOAD_RETRY_SLEEP = 5.seconds
 
     # Generous timeouts, overriden for big files upload/download
     HTTP_CONNECTION_TIMEOUT = 60
@@ -121,7 +123,7 @@ module CartoDB
         response = http_client.put(api_url(action: 'cancel'),
                                    connecttimeout: HTTP_CONNECTION_TIMEOUT,
                                    timeout: HTTP_REQUEST_TIMEOUT)
-        if response.response_code == 400 && message =~ /CANNOT CANCEL THE COMPLETED, DELETED, FAILED OR ALREADY CANCELLED JOB/
+        if is_cancellable?(response)
           @log.append_and_store "Job was already cancelled"
         else
           handle_api_error(response)
@@ -149,43 +151,59 @@ module CartoDB
     def result
       return @result unless @result.nil?
 
-      raise 'No request_id provided' unless request_id
-      results_filename = File.join(dir, "#{request_id}.zip")
+      raise 'No request_id provided' unless @geocoding_model.remote_id
+      results_filename = File.join(dir, "#{@geocoding_model.remote_id}.zip")
       download_url = api_url({}, 'result')
-      # generous timeout for download of results
-      request = http_client.request(download_url,
+      download_status_code = nil
+      retries = 0
+      while true
+        if(!download_status_code.nil? && download_status_code == 200)
+          break
+        elsif !download_status_code.nil? && download_status_code == 404
+          # 404 means that the results file is not ready yet
+          sleep DOWLOAD_RETRY_SLEEP
+          retries += 1
+        elsif retries >= DOWNLOAD_RETRIES
+          raise 'Download request failed: Too many retries, should be a problem with HERE servers'
+        elsif !download_status_code.nil? && download_status_code > 200 && download_status_code != 404
+          raise "Download request failed: Http status code #{download_status_code}"
+        end
+        download_status_code = execute_results_request(download_url, results_filename)
+      end
+      @result = results_filename
+    end
+
+
+    private
+
+    def execute_results_request(download_url, results_filename)
+        download_status_code = nil
+        # generous timeout for download of results
+        request = http_client.request(download_url,
                                     method: :get,
                                     timeout: 5.hours)
 
-      File.open(results_filename, 'wb') do |download_file|
-
-        request.on_headers do |response|
-          if response.success? == false
-            # TODO: better error handling
-            raise 'Download request failed'
+        File.open(results_filename, 'wb') do |download_file|
+          request.on_headers do |response|
+            download_status_code = response.response_code
           end
-        end
 
-        request.on_body do |chunk|
-          download_file.write(chunk)
-        end
+          request.on_body do |chunk|
+            if download_status_code == 200
+              download_file.write(chunk)
+            end
+          end
 
-        request.on_complete do |response|
-          if response.success? == false
-            # TODO: better error handling
-            raise 'Download request failed'
+          request.on_complete do |response|
+            download_status_code = response.response_code
           end
-        end
 
-        request.run
-      end
+          request.run
+        end
 
-      @result = results_filename
+        return download_status_code
     end
 
-
-    private
-
     def config
       GeocoderConfig.instance.get
     end
@@ -289,5 +307,11 @@ module CartoDB
         @geocoding_model.save
       end
     end
+
+    def is_cancellable?(response)
+      message = extract_response_field(response.body, '//Details')
+      response.response_code == 400 && message =~ /CANNOT CANCEL THE COMPLETED, DELETED, FAILED OR ALREADY CANCELLED JOB/
+    end
+
   end
 end

services/geocoder/spec/hires_batch_geocoder_spec.rb

@@ -173,6 +173,8 @@ END_XML
 
     it 'downloads the result file from the remote server' do
       request_id = 'dummy_request_id'
+      @geocoding_model.remote_id = request_id
+      @geocoding_model.save
       @batch_geocoder.stubs(:request_id).returns(request_id)
       expected_response_body = 'dummy result file contents'
       url = @batch_geocoder.send(:api_url, {}, 'result')
@@ -189,6 +191,8 @@ END_XML
 
     it 'raises an exception if cannot get a result file' do
       request_id = 'dummy_request_id'
+      @geocoding_model.remote_id = request_id
+      @geocoding_model.save
       @batch_geocoder.stubs(:request_id).returns(request_id)
       expected_response_body = 'dummy result file contents'
       url = @batch_geocoder.send(:api_url, {}, 'result')

services/table-geocoder/lib/table_geocoder.rb

@@ -28,7 +28,7 @@ module CartoDB
         connection:  connection,
         formatter:   clean_formatter,
         sql_api:     arguments[:cache],
-        working_dir: working_dir,
+        working_dir: @working_dir,
         table_name:  table_name,
         qualified_table_name: @qualified_table_name,
         max_rows:    @max_rows,
@@ -46,7 +46,7 @@ module CartoDB
       # Sync state because cancel is made synchronous
       @geocoding_model.refresh
       if not @geocoding_model.cancelled?
-        process_results if geocoder.status == 'completed'
+        process_results if @geocoding_model.state == 'completed'
         cache.store unless cache_disabled?
       end
     ensure
@@ -118,7 +118,7 @@ module CartoDB
 
     # Generate a csv input file from the geocodable rows
     def generate_csv
-      csv_file = File.join(working_dir, "wadus.csv")
+      csv_file = File.join(@working_dir, "wadus.csv")
       # INFO: we exclude inputs too short and "just digits" inputs, which will remain as georef_status = false
       query = %Q{
         WITH geocodable AS (
@@ -145,9 +145,9 @@ module CartoDB
 
     def deflate_results
       current_directory = Dir.pwd
-      Dir.chdir(working_dir)
+      Dir.chdir(@working_dir)
       out = `unp *.zip 2>&1`
-      out = `unp #{working_dir}/*_out.zip 2>&1`
+      out = `unp #{@working_dir}/*_out.zip 2>&1`
     ensure
       Dir.chdir(current_directory)
     end
@@ -194,7 +194,7 @@ module CartoDB
     end
 
     def deflated_results_path
-      Dir[File.join(working_dir, '*_out.txt')][0]
+      Dir[File.join(@working_dir, '*_out.txt')][0]
     end
 
     def update_metrics