-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: do not overwrite comet config when set in InterceptConfigsPreRunHandler
#16395
Conversation
This comment has been minimized.
This comment has been minimized.
if conf.Consensus.TimeoutCommit == defaultCometCfg.Consensus.TimeoutCommit { | ||
conf.Consensus.TimeoutCommit = 5 * time.Second | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand this. This is what it's doing:
if conf.Consensus.TimeoutCommit == 1s {
conf.Consensus.TimeoutCommit = 5s
}
This other seems unnecessary because that's the default value for RecvRate
, so it's ineffective:
if conf.P2P.RecvRate == defaultCometCfg.P2P.RecvRate {
conf.P2P.RecvRate = 5120000
}
Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is what it was always doing, so I kept the same behavior.
The issue is when you purposely change the timeout config in intercept pre handler, then it was getting overwritten here.
Making the override made by the app dev useless.
So this just checks if you didn't change the config before, so the SDK stays opinionated if the app dev hasn't changed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the unnecessary override and added a comment.
…nHandler` (backport cosmos#16395) (cosmos#16399) Co-authored-by: Julien Robert <julien@rbrt.fr>
…nHandler` (backport cosmos#16395) (cosmos#16399) Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
The SDK seems opinionated about some comet config parameters.
However, it renders the override of those parameters by an app useless and with no feedback.
This adds a check that keeps the SDK opinionated if the value is the default comet value.
Which allows app devs to overwrite those value if they want to.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change