Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Hack -> Config: add options to disable smart retry #281

Merged
merged 3 commits into from
Sep 1, 2017
Merged

Conversation

kobeyang
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage increased (+0.2%) to 67.489% when pulling eac12a9 on smart_retry into 2a2575a on master.

@kobeyang kobeyang requested review from thuningxu and kirg August 25, 2017 18:26
Copy link
Contributor

@kirg kirg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish we could make the default to be "smart-retry" disabled. And if we go that route we should rename the flag to "FlagEnableSmartRetry", etc.

The only trouble is for existing CGs, we will need to set the flag to 'true', because the behaviour is that if the owner-email does not have "+smartRetryDisable", then it is enabled! Is that doable?

if disableNackThrottling == "true" {

disableNackThrottling := c.Bool(common.FlagDisableNackThrottling)
if disableNackThrottling == true {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could probably simply do:

if c.Bool(common.FlagDisableNackThrottling {
   ...

instead of assigning it to a separate variable just for checking, etc.

options[common.FlagDisableNackThrottling] = "true"
}

disableSmartRetry := c.Bool(common.FlagDisableSmartRetry)
if disableSmartRetry == true {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

if c.Bool(common.FlagDisableSmartRetry) == true {
disableSmartRetry = "true"
}

options = make(map[string]string)
options[common.FlagDisableNackThrottling] = disableNackThrottling
Copy link
Contributor

@kirg kirg Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't you set this with one if-check?

if c.Bool(common.FlagDisableNackThrottling) {
    options[common.FlagDisableNackThrottling] = "true"
}

etc

@coveralls
Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage decreased (-0.1%) to 67.197% when pulling 74cf8f9 on smart_retry into 2a2575a on master.

@coveralls
Copy link

coveralls commented Aug 31, 2017

Coverage Status

Coverage increased (+0.09%) to 67.415% when pulling 00da29b on smart_retry into 2a2575a on master.

@kobeyang kobeyang merged commit c293949 into master Sep 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants