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

Pause: Circuit Breaker 1 #2497

Closed
wants to merge 5 commits into from

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Oct 15, 2018

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

Dependent on: #2491

Addresses: #3234


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mossid mossid added the wip label Oct 15, 2018
@mossid mossid force-pushed the joon/926-circuit-breaker branch 3 times, most recently from 9d57b86 to 90fb578 Compare October 22, 2018 10:58
@mossid mossid mentioned this pull request Oct 23, 2018
5 tasks
@mossid mossid changed the title WIP: Circuit Breaker R4R: Circuit Breaker Oct 23, 2018
@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (joon/paramstore-subkey@078aead). Click here to learn what that means.
The diff coverage is 43.56%.

@@                    Coverage Diff                    @@
##             joon/paramstore-subkey    #2497   +/-   ##
=========================================================
  Coverage                          ?   53.17%           
=========================================================
  Files                             ?      132           
  Lines                             ?     9690           
  Branches                          ?        0           
=========================================================
  Hits                              ?     5153           
  Misses                            ?     4224           
  Partials                          ?      313

@alexanderbez
Copy link
Contributor

alexanderbez commented Oct 23, 2018

@mossid can we separate out the changes to the param store into a separate PR?

@sunnya97
Copy link
Member

Why do we need a new module for this instead adding it to the current AnteHandler?

@cwgoes
Copy link
Contributor

cwgoes commented Oct 25, 2018

Should we write a spec for the circuit breaker (even this non-governance part)?

@mossid
Copy link
Contributor Author

mossid commented Oct 26, 2018

@sunnya97 auth module is mainly for account verification including nonce and signature verification, so I think it is better to separate out to another module since it is for verifying module's availability.

@mossid mossid mentioned this pull request Oct 26, 2018
5 tasks
@alexanderbez
Copy link
Contributor

@mossid is there a spec for this I can look at? It'll help me review the code better.

@mossid mossid changed the base branch from develop to joon/paramstore-subkey October 26, 2018 15:21
@ValarDragon
Copy link
Contributor

Ideally we want composability of ante handlers, so this would make sense as a composition on top of our current ante handler. We probably don't have composability atm though.

@cwgoes
Copy link
Contributor

cwgoes commented Dec 12, 2018

@mossid Checking in on this PR - #2491 has since been merged - should we rebase this on develop? Can we write a spec or at least an issue describing what this PR intends to accomplish?

@mossid mossid changed the title R4R: Circuit Breaker R4R: Circuit Breaker 1 Jan 5, 2019
@alexanderbez
Copy link
Contributor

It seems this branch is severely out of date :(

@cwgoes
Copy link
Contributor

cwgoes commented Jan 11, 2019

Needs to be rebased and retargeted against develop if we want to consider merging this.

@rigelrozanski
Copy link
Contributor

Can we rename to Pause State or "Pausing" - wherever relevant? thoughts? (discussed here #2935)

@jackzampolin
Copy link
Member

I think we need to circle up as team and decide what we want out of this feature and write some spec on it.

@mossid mossid changed the title R4R: Circuit Breaker 1 Pause: Circuit Breaker 1 Jan 30, 2019
@mossid mossid closed this Mar 18, 2019
@mircea-c mircea-c deleted the joon/926-circuit-breaker branch May 3, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants