Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reprovider: add config option to set reprovide interval #3101

Merged
merged 2 commits into from
Aug 20, 2016

Conversation

whyrusleeping
Copy link
Member

License: MIT
Signed-off-by: Jeromy [email protected]

@whyrusleeping whyrusleeping added the status/in-progress In progress label Aug 19, 2016
License: MIT
Signed-off-by: Jeromy <[email protected]>
@Kubuxu
Copy link
Member

Kubuxu commented Aug 20, 2016

LGTM

@Kubuxu Kubuxu added RFM and removed status/in-progress In progress need/review Needs a review labels Aug 20, 2016
@whyrusleeping whyrusleeping merged commit eac1c63 into master Aug 20, 2016
@whyrusleeping whyrusleeping deleted the feat/reprovide-config branch August 20, 2016 19:14
interval = dur
}

go n.Reprovider.ProvideEvery(ctx, interval)
Copy link
Member

Choose a reason for hiding this comment

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

  • i wonder if this will have serious adverse effects by triggering or not triggering a whole large codepath.
  • I think this may be more resistant to error and code modification if we always do go n.Reprovider.ProvideEvery(ctx, interval) instead of guarding it in an if clause. The func can treats 0 (or a negative number) as "oh, do nothing? ok and it either waits or exits". Your code becomes:
interval := kReprovideFrequency
if cfg.Reprovider.Interval != "" {
  dur, err := time.ParseDuration(cfg.Reprovider.Interval)
  if err != nil {
    return err
  }
  interval = dur
}
go n.Reprovider.ProvideEvery(ctx, interval)

And you always guard in n.Reprovider.ProvideEvery which should happen already.

@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants