-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Params, Clients] add generic ParamsQuerier
#995
base: issues/543/cache/memory
Are you sure you want to change the base?
[Params, Clients] add generic ParamsQuerier
#995
Conversation
48f5762
to
49183d7
Compare
49183d7
to
ba61d14
Compare
011d2d7
to
d5ce62f
Compare
// P is a pointer type of the parameters, and Q is the interface type of the | ||
// corresponding query client. | ||
type cachedParamsQuerier[P cosmostypes.Msg, Q paramsQuerierIface[P]] struct { | ||
clientConn gogogrpc.ClientConn |
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.
Is this needed at the cachedParamsQuerier
level?
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.
Yes, indeed. The cachedParamsQuerier
has to hold a reference to this because it's a dependency of all query clients, and it also has to know how to generically construct a given modules query client. See NewCachedParamsQuerier()
:
if err = depinject.Inject(
deps,
&querier.clientConn,
); err != nil {
return nil, err
}
querier.queryClient = queryClientConstructor(querier.clientConn)
} | ||
|
||
// Update the cache. | ||
if err = bq.paramsCache.Set("params", params); err != nil { |
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.
We should consider the case where paramsCache
is in historical
mode, otherwise this call might fail.
Additionally, I think we should find a way to get the height/version of the retrieved params in order to set it using SetAsOfVersion
.
Maybe add the height
to the QueryParamsResponse
?
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.
As discussed:
cachedParamsQuerier
's cache is ALWAYS in historical mode.- This isn't possible anymore due to the decision we made to disable
#Set()
in historical mode. - We're going to transition to event-driven cache warming.
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.
We're going to transition to event-driven cache warming.
Can you add a TODO for that somewhere?
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.
cachedParamsQuerier's cache is ALWAYS in historical mode.
#PUC
Co-authored-by: Redouane Lakrache <r3d0ne@gmail.com>
fbd5934
to
3abc4b5
Compare
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.
Nothing big. Appreciate all the TODOs, comments and explanations.
Will approve after we merge in the base!
@@ -380,3 +381,15 @@ type HistoricalQueryCache[T any] interface { | |||
// SetAsOfVersion adds or updates a value at a specific version number. | |||
SetAsOfVersion(key string, value T, version int64) error | |||
} | |||
|
|||
// ParamsQuerier represents a generic querier for module parameters. | |||
// This interface should be implemented by any module-specific querier |
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.
// This interface should be implemented by any module-specific querier | |
// This interface SHOULD be implemented by any module-specific querier |
defaultMaxVersionAge = 100 | ||
defaultMaxKeys = 1000 |
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.
defaultMaxVersionAge = 100 | |
defaultMaxKeys = 1000 | |
// These are sensible defaults that can be customized below. | |
// The default oldest version (relative to the latest) that'll be considered | |
// a cache hit by the client. | |
defaultMaxVersionAge = 100 | |
// The default maximum number of (key, value) pairs that'll be maintained | |
// in the cache irrespective of the key or value size. | |
defaultMaxKeys = 1000 |
return func(cfg *paramsQuerierConfig) { | ||
cfg.logger = logger | ||
} | ||
} |
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.
This is clean!
} | ||
|
||
// Update the cache. | ||
if err = bq.paramsCache.Set("params", params); err != nil { |
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.
We're going to transition to event-driven cache warming.
Can you add a TODO for that somewhere?
} | ||
|
||
// Update the cache. | ||
if err = bq.paramsCache.Set("params", params); err != nil { |
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.
cachedParamsQuerier's cache is ALWAYS in historical mode.
#PUC
err = cache.ErrCacheMiss.Wrapf("TODO: on-chain historical data not implemented") | ||
logger.Error().Msgf("%s", err) | ||
|
||
// Meanwhile, return current params as fallback. 😬 |
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.
Summary
Adds the the
ParamsQuerier
interface,cachedParamsQuerier
implementation, config, and options functions.ParamsQuerier
is a generic implementation of a "params querier" (i.e.interface { GetParams(ctx) (P, error }
, whereP
is the params type) that can be embedded in any module-specific query client implementation. It implements configurable params caching for historical heights (incl. current height).Issue
Type of change
Select one or more from the following:
consensus-breaking
label if so. See [Infra] Automatically add theconsensus-breaking
label #791 for detailsTesting
make docusaurus_start
; only needed if you make doc changesmake go_develop_and_test
make test_e2e
devnet-test-e2e
label to the PR.Sanity Checklist