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

Implement caching for cacheable for short time methods #69

Closed
wants to merge 1 commit into from

Conversation

evgeniy-scherbina
Copy link
Contributor

No description provided.

@evgeniy-scherbina evgeniy-scherbina force-pushed the yevhenii/cache branch 2 times, most recently from 3a64104 to f79494e Compare November 10, 2023 17:14
@evgeniy-scherbina evgeniy-scherbina changed the title Temporary Commit Implement caching for cacheable for short time methods Nov 10, 2023
Copy link
Contributor

@galxy25 galxy25 left a comment

Choose a reason for hiding this comment

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

code looks good and tests pass on my machine locally - please update docs with info on new category of methods being cached / strategy

@@ -310,6 +312,7 @@ func ReadConfig() Config {
CacheMethodHasBlockHashParamTTL: time.Duration(EnvOrDefaultInt(CACHE_METHOD_HAS_BLOCK_HASH_PARAM_TTL_ENVIRONMENT_KEY, 0)) * time.Second,
CacheStaticMethodTTL: time.Duration(EnvOrDefaultInt(CACHE_STATIC_METHOD_TTL_ENVIRONMENT_KEY, 0)) * time.Second,
CacheMethodHasTxHashParamTTL: time.Duration(EnvOrDefaultInt(CACHE_METHOD_HAS_TX_HASH_PARAM_TTL_ENVIRONMENT_KEY, 0)) * time.Second,
CacheMethodCacheableForShortTimeTTL: time.Duration(EnvOrDefaultInt(CACHE_METHOD_CACHEABLE_FOR_SHORT_TIME_TTL_ENVIRONMENT_KEY, 0)) * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this (and all other caching ttls, having a non-zero default ttl (say 1 second) makes more sense, otherwise default behavior with zero operator / environment file config is caching is turned off (even if feature flag for caching itself is enabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@galxy25 closing this according to our latest discussion with Kevin and Nick

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