Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Caching POA #1454

Merged
merged 8 commits into from May 10, 2019
Merged

Caching POA #1454

merged 8 commits into from May 10, 2019

Conversation

dkulic
Copy link
Contributor

@dkulic dkulic commented Feb 5, 2019

Signed-off-by: Darko Kulic darko.kulic@evernym.com

Signed-off-by: Darko Kulic <darko.kulic@evernym.com>
pub extern fn indy_get_cred_def(command_handle: IndyHandle,
pool_handle: IndyHandle,
wallet_handle: IndyHandle,
submitter_did: *const c_char,

Choose a reason for hiding this comment

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

  1. I suggest to remove submitter_did param as it can be hard-coded or auto-generated for each request
  2. Do we need to return cache timestamp also?
  3. I suggest to add options_json that will allow to:
  • Skip caching
  • Provide required freshness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Skip caching have sense in a meaning do not store retrieved value inside of cache.
    For freshness, I think that schema and credential_def cannot be updated (without change of id) so freshness is not needed. For that reason I do not see need for cache timestamp.

Choose a reason for hiding this comment

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

In my understanding CredDef and Schema can be updated. Schema can get backward compatible changes. Keys in CredDef can be rotated in some cases. Also some entities we plan to cache like DID Doc can be updated much often. @ashcherbakov What do you think?

dkulic and others added 2 commits March 6, 2019 17:04

Several methods may be implemented for purging the cached data:

#### Purge all
Copy link

Choose a reason for hiding this comment

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

I believe we need to decide what exact option to use or provide an API for configuration of cache. For example, we can add behavior configuration to indy_init() method.

Copy link

Choose a reason for hiding this comment

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

To define purge behavior i also suggest to look to http cache options:

Cache-Control: max-age=<seconds>
Cache-Control: max-stale[=<seconds>]
Cache-Control: min-fresh=<seconds>
Cache-Control: no-cache 
Cache-Control: no-store
Cache-Control: no-transform
Cache-Control: only-if-cached

In this case we can define exact behavior for each entity.

Copy link

Choose a reason for hiding this comment

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

@dkulic @jovfer @ashcherbakov @dhh1128 What do you think on ^?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I follow your thoughts regarding defining exact behavior for each entity. Do you mean adding specific caching policy to each entity, so that purging process may selectively purge only entities which satisfy its own policy? If this is the case, that seams to complicated...

Copy link

@vimmerru vimmerru Mar 7, 2019

Choose a reason for hiding this comment

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

@dkulic The idea is very simple:

  1. When app asks for some entity it can explicitly say how long it want to store this entity in cache
  2. On start app can call purge method that will remove all outdated records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds doable. :)

Copy link

Choose a reason for hiding this comment

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

also purge can have force param that will remove all cache records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the timeout idea, because you may lock in some cache items in cache forever. (eg. so you may use your passport on foreign airport, without internet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some thoughts I figured out I do not like idea about purging on start/specifying ttl for every entity. I would expect that normally you would want data to be in cache for longer time, so you may during request choose max-age of data you want. I would modify it like this:

  1. When requesting a data, app may use some cache control options (like max-age, no-cache, no-store, only-if-cached, for eg.)
  2. purge method would have option of specifying max-age (or similar), so older data would be deleted.

/// id: identifier of credential definition.
/// options_json:
/// {
/// forceUpdate: (optional, false by default) Force update of record in cache from the ledger,
Copy link

Choose a reason for hiding this comment

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

I believe we can use something similar to http-cache control headers:

Cache-Control: max-age=<seconds>
Cache-Control: max-stale[=<seconds>]
Cache-Control: min-fresh=<seconds>
Cache-Control: no-cache 
Cache-Control: no-store
Cache-Control: no-transform
Cache-Control: only-if-cached

Copy link
Contributor Author

@dkulic dkulic Mar 7, 2019

Choose a reason for hiding this comment

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

Some of these options look very useful. I would like not to have too many options, which may be too confusing. I especially like: max-age, only-if-cached.

@jovfer
Copy link
Contributor

jovfer commented Mar 13, 2019

@dkulic @vimmerru what is the next step for this PoA?

dkulic and others added 2 commits April 18, 2019 11:40
Signed-off-by: Darko Kulic <darko.kulic@evernym.com>
Copy link
Contributor

@jovfer jovfer left a comment

Choose a reason for hiding this comment

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

I like the latest changes around cache lifetime. I'm voting to merge this design, implement it in code but mark experimental for the next one release

@jovfer jovfer merged commit 06ac842 into hyperledger-archives:master May 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants