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

store Trace refactor #2824

Closed
jaekwon opened this issue Nov 15, 2018 · 2 comments
Closed

store Trace refactor #2824

jaekwon opened this issue Nov 15, 2018 · 2 comments
Assignees

Comments

@jaekwon
Copy link
Contributor

jaekwon commented Nov 15, 2018

  • (rms/cms).WithTracingContext() and .WithTracer() are misnomers, they should be .Set*(). Comments are either ambiguous "A MultiStore is returned"... (which one?), or wrong "It returns a modified MultiStore"... (it returns the same multistore and also happens to modify it in place).

  • (rms/cms).CacheWrap().WithTracingContext() modifies the traceContext dict of the orignal/parent MultiStore, which doesn't make sense. A CacheWrap()'d thing should not affect the wrapped thing (in general, until .Write()). In terms of a "trace context", it should probably never modify the parent's "trace context".

  • cms.WithTracingContext() doesn’t always apply the tracing context until ms.CacheWrap/CacheMultiStore is called, whereas rms.WithTracingContext() does (e.g. rms.GetKVStore() uses the new tracing context, whereas cms.GetKVStore() does not, unless the cms itself was constructed with a tracing context first from the parent MultiStore, in which case two wrongs make a "right" because of the earlier mentioned issue of parent-tracing-context-overwriting). And otherwise, (rms/cms).CacheWrapWithTrace() doesn't use its args at all.

The fix to all of this, I think, is:

  • Implement TracingMultiStore and use it in (rms/cms).CacheWrapWithTrace().
  • Remove .WithTracingContext/.WithTracer/.TracingEnabled/.ResetTraceContext.
@cwgoes
Copy link
Contributor

cwgoes commented Nov 30, 2018

cc @mossid Should this be integrated with your store refactor PR (#2344)?

@mossid mossid mentioned this issue Dec 3, 2018
@cwgoes cwgoes mentioned this issue Dec 11, 2018
5 tasks
@cwgoes cwgoes assigned mossid and unassigned alexanderbez and cwgoes Jan 21, 2019
@cwgoes
Copy link
Contributor

cwgoes commented Jan 21, 2019

#2985 will close this I think.

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this issue Mar 1, 2024
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

No branches or pull requests

5 participants