From aa0d7e8d17a78dd484fbb745e1971d0f399bfab2 Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Fri, 7 Oct 2016 16:28:02 -0700 Subject: [PATCH 1/7] exponentially back off sleep if the Octokit::Forbidden error is seen --- lib/github_changelog_generator/octo_fetcher.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index aa56d01..27ee894 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -280,19 +280,35 @@ Make sure, that you push tags to remote repo via 'git push --tags'" # # @return [Object] returns exactly the same, what you put in the block, but wrap it with begin-rescue block def check_github_response + attempt = 1 begin value = yield rescue Octokit::Unauthorized => e Helper.log.error e.message abort "Error: wrong GitHub token" rescue Octokit::Forbidden => e + attempt += 1 + sleep_time = exp_backoff(attempt) + Helper.log.warn("sleeping #{sleep_time} second") + sleep(sleep_time) + Helper.log.warn e.message Helper.log.warn GH_RATE_LIMIT_EXCEEDED_MSG Helper.log.warn @client.rate_limit + + retry end value end + # Returns the exponential backoff (seconds) for this attempt number + # + # @param [Integer] attempt the attempt number + # @return [Integer] Exponential backoff seconds + def exp_backoff(attempt) + (2 ** attempt - 1) / 2 + end + # Print specified line on the same string # # @param [String] log_string From 074b46725115f6c5db3e9ac2d6eb5ce7d9a6b163 Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Fri, 7 Oct 2016 16:58:38 -0700 Subject: [PATCH 2/7] added specs and refactor a bit --- .../octo_fetcher.rb | 18 ++++++++++--- spec/unit/octo_fetcher_spec.rb | 26 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index 27ee894..6d0db33 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -8,6 +8,7 @@ module GitHubChangelogGenerator class OctoFetcher PER_PAGE_NUMBER = 100 MAX_THREAD_NUMBER = 25 + MAX_FORBIDDEN_RETRIES = 100 CHANGELOG_GITHUB_TOKEN = "CHANGELOG_GITHUB_TOKEN" GH_RATE_LIMIT_EXCEEDED_MSG = "Warning: Can't finish operation: GitHub API rate limit exceeded, change log may be " \ "missing some issues. You can limit the number of issues fetched using the `--max-issues NUM` argument." @@ -285,22 +286,33 @@ Make sure, that you push tags to remote repo via 'git push --tags'" value = yield rescue Octokit::Unauthorized => e Helper.log.error e.message - abort "Error: wrong GitHub token" + sys_abort("Error: wrong GitHub token") rescue Octokit::Forbidden => e attempt += 1 sleep_time = exp_backoff(attempt) Helper.log.warn("sleeping #{sleep_time} second") - sleep(sleep_time) + sys_sleep(sleep_time) Helper.log.warn e.message Helper.log.warn GH_RATE_LIMIT_EXCEEDED_MSG Helper.log.warn @client.rate_limit - retry + if attempt >= MAX_FORBIDDEN_RETRIES + sys_abort("Exceeded retry limit") + else + retry + end end value end + def sys_sleep(seconds) + sleep(seconds) + end + + def sys_abort(msg) + abort(msg) + end # Returns the exponential backoff (seconds) for this attempt number # # @param [Integer] attempt the attempt number diff --git a/spec/unit/octo_fetcher_spec.rb b/spec/unit/octo_fetcher_spec.rb index f74e25f..14dd244 100644 --- a/spec/unit/octo_fetcher_spec.rb +++ b/spec/unit/octo_fetcher_spec.rb @@ -12,6 +12,32 @@ describe GitHubChangelogGenerator::OctoFetcher do let(:fetcher) { GitHubChangelogGenerator::OctoFetcher.new(options) } + describe "#check_github_response" do + context "when returns successfully" do + it "returns block value" do + expect(fetcher.send(:check_github_response) { 1+1 }).to eq(2) + end + end + + context "when raises Octokit::Unauthorized" do + it "aborts" do + expect(fetcher).to receive(:sys_abort).with("Error: wrong GitHub token") + fetcher.send(:check_github_response) { raise(Octokit::Unauthorized) } + end + end + + context "when raises Octokit::Forbidden" do + it "sleeps and retries and then aborts" do + retry_limit = GitHubChangelogGenerator::OctoFetcher::MAX_FORBIDDEN_RETRIES - 1 + + expect(fetcher).to receive(:sys_abort).with("Exceeded retry limit") + expect(fetcher).to receive(:sys_sleep).exactly(retry_limit).times + fetcher.send(:check_github_response) { raise(Octokit::Forbidden) } + end + end + + end + describe "#fetch_github_token" do token = GitHubChangelogGenerator::OctoFetcher::CHANGELOG_GITHUB_TOKEN context "when token in ENV exist" do From 7b8c6674476036ce10650e67fd8975114c15edd6 Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Fri, 7 Oct 2016 18:37:36 -0700 Subject: [PATCH 3/7] style --- lib/github_changelog_generator/octo_fetcher.rb | 2 +- spec/unit/octo_fetcher_spec.rb | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index 6d0db33..c76affe 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -318,7 +318,7 @@ Make sure, that you push tags to remote repo via 'git push --tags'" # @param [Integer] attempt the attempt number # @return [Integer] Exponential backoff seconds def exp_backoff(attempt) - (2 ** attempt - 1) / 2 + (2**attempt - 1) / 2 end # Print specified line on the same string diff --git a/spec/unit/octo_fetcher_spec.rb b/spec/unit/octo_fetcher_spec.rb index 14dd244..7a254b4 100644 --- a/spec/unit/octo_fetcher_spec.rb +++ b/spec/unit/octo_fetcher_spec.rb @@ -15,7 +15,7 @@ describe GitHubChangelogGenerator::OctoFetcher do describe "#check_github_response" do context "when returns successfully" do it "returns block value" do - expect(fetcher.send(:check_github_response) { 1+1 }).to eq(2) + expect(fetcher.send(:check_github_response) { 1 + 1 }).to eq(2) end end @@ -35,7 +35,6 @@ describe GitHubChangelogGenerator::OctoFetcher do fetcher.send(:check_github_response) { raise(Octokit::Forbidden) } end end - end describe "#fetch_github_token" do From 4f9cfdd3375f8c1d5128ef8ad8e974a3d40fe4bd Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Fri, 7 Oct 2016 18:50:58 -0700 Subject: [PATCH 4/7] spacing --- lib/github_changelog_generator/octo_fetcher.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index c76affe..ee82e07 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -313,6 +313,7 @@ Make sure, that you push tags to remote repo via 'git push --tags'" def sys_abort(msg) abort(msg) end + # Returns the exponential backoff (seconds) for this attempt number # # @param [Integer] attempt the attempt number From 77ecafa978e8d2f1f91a4686ee31df3fb85870f5 Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Wed, 19 Oct 2016 14:30:43 -0700 Subject: [PATCH 5/7] using retriable gem --- github_changelog_generator.gemspec | 2 + .../octo_fetcher.rb | 65 ++++++++++--------- spec/unit/octo_fetcher_spec.rb | 2 +- 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/github_changelog_generator.gemspec b/github_changelog_generator.gemspec index c114b9c..ec22619 100644 --- a/github_changelog_generator.gemspec +++ b/github_changelog_generator.gemspec @@ -29,4 +29,6 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency("octokit", ["~> 4.0"]) spec.add_runtime_dependency("faraday-http-cache") spec.add_runtime_dependency("activesupport") + spec.add_runtime_dependency("retriable", ["~> 2.1"]) + end diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index ee82e07..a1f1f0f 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require 'retriable' module GitHubChangelogGenerator # A Fetcher responsible for all requests to GitHub and all basic manipulation with related data # (such as filtering, validating, e.t.c) @@ -281,47 +282,47 @@ Make sure, that you push tags to remote repo via 'git push --tags'" # # @return [Object] returns exactly the same, what you put in the block, but wrap it with begin-rescue block def check_github_response - attempt = 1 - begin - value = yield - rescue Octokit::Unauthorized => e - Helper.log.error e.message - sys_abort("Error: wrong GitHub token") - rescue Octokit::Forbidden => e - attempt += 1 - sleep_time = exp_backoff(attempt) - Helper.log.warn("sleeping #{sleep_time} second") - sys_sleep(sleep_time) - - Helper.log.warn e.message - Helper.log.warn GH_RATE_LIMIT_EXCEEDED_MSG - Helper.log.warn @client.rate_limit - - if attempt >= MAX_FORBIDDEN_RETRIES - sys_abort("Exceeded retry limit") - else - retry - end + Retriable.retriable(retry_options) do + yield end - value + + rescue Octokit::Forbidden => e + Helper.log.error("#{e.class}: #{e.message}") + sys_abort("Exceeded retry limit") + rescue Octokit::Unauthorized => e + Helper.log.error("#{e.class}: #{e.message}") + sys_abort("Error: wrong GitHub token") end - def sys_sleep(seconds) - sleep(seconds) + # Exponential backoff + def retry_options + { + :on => [Octokit::Forbidden], + :tries => MAX_FORBIDDEN_RETRIES, + :base_interval => sleep_base_interval, + :multiplier => 1.0, + :rand_factor => 0.0, + :on_retry => retry_callback + } + end + + def sleep_base_interval + 1.0 + end + + def retry_callback + Proc.new do |exception, try, elapsed_time, next_interval| + Helper.log.warn("RETRY - #{exception.class}: '#{exception.message}'") + Helper.log.warn("#{try} tries in #{elapsed_time} seconds and #{next_interval} seconds until the next try") + Helper.log.warn GH_RATE_LIMIT_EXCEEDED_MSG + Helper.log.warn @client.rate_limit + end end def sys_abort(msg) abort(msg) end - # Returns the exponential backoff (seconds) for this attempt number - # - # @param [Integer] attempt the attempt number - # @return [Integer] Exponential backoff seconds - def exp_backoff(attempt) - (2**attempt - 1) / 2 - end - # Print specified line on the same string # # @param [String] log_string diff --git a/spec/unit/octo_fetcher_spec.rb b/spec/unit/octo_fetcher_spec.rb index 7a254b4..a4398b1 100644 --- a/spec/unit/octo_fetcher_spec.rb +++ b/spec/unit/octo_fetcher_spec.rb @@ -29,9 +29,9 @@ describe GitHubChangelogGenerator::OctoFetcher do context "when raises Octokit::Forbidden" do it "sleeps and retries and then aborts" do retry_limit = GitHubChangelogGenerator::OctoFetcher::MAX_FORBIDDEN_RETRIES - 1 + allow(fetcher).to receive(:sleep_base_interval).and_return(0) expect(fetcher).to receive(:sys_abort).with("Exceeded retry limit") - expect(fetcher).to receive(:sys_sleep).exactly(retry_limit).times fetcher.send(:check_github_response) { raise(Octokit::Forbidden) } end end From 15d0b63b45e8d999dad22f3382047236ea5cf679 Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Wed, 19 Oct 2016 14:33:05 -0700 Subject: [PATCH 6/7] assert exactly x times --- spec/unit/octo_fetcher_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/octo_fetcher_spec.rb b/spec/unit/octo_fetcher_spec.rb index a4398b1..f2dbdeb 100644 --- a/spec/unit/octo_fetcher_spec.rb +++ b/spec/unit/octo_fetcher_spec.rb @@ -29,7 +29,7 @@ describe GitHubChangelogGenerator::OctoFetcher do context "when raises Octokit::Forbidden" do it "sleeps and retries and then aborts" do retry_limit = GitHubChangelogGenerator::OctoFetcher::MAX_FORBIDDEN_RETRIES - 1 - allow(fetcher).to receive(:sleep_base_interval).and_return(0) + allow(fetcher).to receive(:sleep_base_interval).exactly(retry_limit).times.and_return(0) expect(fetcher).to receive(:sys_abort).with("Exceeded retry limit") fetcher.send(:check_github_response) { raise(Octokit::Forbidden) } From 134d10bab026d9e289c99d10384aea28eb95aa3d Mon Sep 17 00:00:00 2001 From: Andrew Waage Date: Thu, 20 Oct 2016 11:49:02 -0700 Subject: [PATCH 7/7] some rubocop updates --- github_changelog_generator.gemspec | 1 - lib/github_changelog_generator/octo_fetcher.rb | 16 ++++++++-------- lib/github_changelog_generator/parser.rb | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/github_changelog_generator.gemspec b/github_changelog_generator.gemspec index ec22619..a621547 100644 --- a/github_changelog_generator.gemspec +++ b/github_changelog_generator.gemspec @@ -30,5 +30,4 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency("faraday-http-cache") spec.add_runtime_dependency("activesupport") spec.add_runtime_dependency("retriable", ["~> 2.1"]) - end diff --git a/lib/github_changelog_generator/octo_fetcher.rb b/lib/github_changelog_generator/octo_fetcher.rb index a1f1f0f..2e68f51 100644 --- a/lib/github_changelog_generator/octo_fetcher.rb +++ b/lib/github_changelog_generator/octo_fetcher.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -require 'retriable' +require "retriable" module GitHubChangelogGenerator # A Fetcher responsible for all requests to GitHub and all basic manipulation with related data # (such as filtering, validating, e.t.c) @@ -297,12 +297,12 @@ Make sure, that you push tags to remote repo via 'git push --tags'" # Exponential backoff def retry_options { - :on => [Octokit::Forbidden], - :tries => MAX_FORBIDDEN_RETRIES, - :base_interval => sleep_base_interval, - :multiplier => 1.0, - :rand_factor => 0.0, - :on_retry => retry_callback + on: [Octokit::Forbidden], + tries: MAX_FORBIDDEN_RETRIES, + base_interval: sleep_base_interval, + multiplier: 1.0, + rand_factor: 0.0, + on_retry: retry_callback } end @@ -311,7 +311,7 @@ Make sure, that you push tags to remote repo via 'git push --tags'" end def retry_callback - Proc.new do |exception, try, elapsed_time, next_interval| + proc do |exception, try, elapsed_time, next_interval| Helper.log.warn("RETRY - #{exception.class}: '#{exception.message}'") Helper.log.warn("#{try} tries in #{elapsed_time} seconds and #{next_interval} seconds until the next try") Helper.log.warn GH_RATE_LIMIT_EXCEEDED_MSG diff --git a/lib/github_changelog_generator/parser.rb b/lib/github_changelog_generator/parser.rb index 9323e46..c62f187 100755 --- a/lib/github_changelog_generator/parser.rb +++ b/lib/github_changelog_generator/parser.rb @@ -42,7 +42,7 @@ module GitHubChangelogGenerator # setup parsing options def self.setup_parser(options) - parser = OptionParser.new do |opts| + parser = OptionParser.new do |opts| # rubocop:disable Metrics/BlockLength opts.banner = "Usage: github_changelog_generator [options]" opts.on("-u", "--user [USER]", "Username of the owner of target GitHub repo") do |last| options[:user] = last