Skip to content

backport: add event subscription options and defaults config (#7930)#9491

Merged
sergio-mena merged 2 commits intotendermint:feature/adr075-backportfrom
mmsqe:feature/adr075-backport-config
Sep 28, 2022
Merged

backport: add event subscription options and defaults config (#7930)#9491
sergio-mena merged 2 commits intotendermint:feature/adr075-backportfrom
mmsqe:feature/adr075-backport-config

Conversation

@mmsqe
Copy link

@mmsqe mmsqe commented Sep 23, 2022

For the full description, see #7930


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@mmsqe mmsqe marked this pull request as ready for review September 23, 2022 14:08
@mmsqe mmsqe requested a review from ebuchman as a code owner September 23, 2022 14:08
@mmsqe mmsqe requested a review from a team September 23, 2022 14:08
@mmsqe
Copy link
Author

mmsqe commented Sep 28, 2022

@sergio-mena may I ask if I could merge the rest backports into one for faster review?

@sergio-mena
Copy link
Contributor

@sergio-mena may I ask if I could merge the rest backports into one for faster review?

Yes, makes sense. We are at the end of the quarter and I can't guarantee a timely review (I'm reviewing this PR now).

However, for the sake of clarity and traceability, please include each backported PR into its own commit in the new PR.
We will not squash your PR once approved, but will rebase it to feature/adr075-backport.
This way we'll get the best of both worlds (reduced back and forth interactions, but keeping traceability of the different changes).

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Thanks for this work

### BREAKING CHANGES

- CLI/RPC/Config
- [config] \#7930 Add new event subscription options and defaults. (@creachadair)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have edited this PR's description to refer to #7930.
So we think it's better that this changelog line refers to #9491.

rpcserver.ReadLimit(config.MaxBodyBytes),
rpcserver.WriteChanCapacity(n.config.RPC.WebSocketWriteBufferSize),
)
wm.SetLogger(wmLogger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, to my comment in line 1133, wouldn't we want to keep this line?

}
}),
rpcserver.ReadLimit(config.MaxBodyBytes),
rpcserver.WriteChanCapacity(n.config.RPC.WebSocketWriteBufferSize),
Copy link
Contributor

Choose a reason for hiding this comment

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

This line wasn't on the original PR, so you might want to keep it after this PR.
From a purely "backporting" point of view, it does make a lot of sense to keep this line.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching, added back.

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

From a backporting point of view, this is ready for merging

@sergio-mena sergio-mena merged commit 069e402 into tendermint:feature/adr075-backport Sep 28, 2022
mmsqe added a commit to mmsqe/tendermint that referenced this pull request Dec 9, 2022
…int#7930) (tendermint#9491)

* config: add event subscription options and defaults (tendermint#7930)

* Apply suggestions from code review

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
mmsqe added a commit to mmsqe/tendermint that referenced this pull request Dec 9, 2022
…int#7930) (tendermint#9491)

* config: add event subscription options and defaults (tendermint#7930)

* Apply suggestions from code review

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
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.

2 participants