Skip to content

Remove file field from CacheFileReference #51520

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

Conversation

DaveCTurner
Copy link
Contributor

CacheFileReference#file is a path to a file that doesn't exist, for use as a
cache key, but whose parent directory is the location of the actual cache file.
This commit replaces it with the path to the cache directory itself, and
computes the cache key when it is needed.

Relates #50693.

`CacheFileReference#file` is a path to a file that doesn't exist, for use as a
cache key, but whose parent directory is the location of the actual cache file.
This commit replaces it with the path to the cache directory itself, and
computes the cache key when it is needed.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jan 28, 2020
@DaveCTurner DaveCTurner requested a review from tlrx January 28, 2020 09:56
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this.

@DaveCTurner DaveCTurner merged commit f2a4957 into elastic:feature/searchable-snapshots Jan 28, 2020
@DaveCTurner DaveCTurner deleted the 2020-01-28-cachefileref-does-not-need-path branch January 28, 2020 11:10
tlrx added a commit that referenced this pull request Jan 31, 2020
Today cache files are identified in cache using a string representing 
an absolute path to a file on disk. This path is a sub directory of the 
current shard data path and as such already contains identification 
bits like the current index id and the shard id. It also contains the 
snapshot id that is passed at CacheDirectory creation time.

While this has been done for quick prototyping and already been 
improved in #51520, it feels wrong to rely on a path converted to
a string as cache keys. Instead we should have a distinct CacheKey 
object to identify CacheFile in cache.

Relates #50693
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants