Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

gps: source cache: interface tweaks and implementation optimizations #1023

Merged
merged 1 commit into from
Aug 17, 2017
Merged

gps: source cache: interface tweaks and implementation optimizations #1023

merged 1 commit into from
Aug 17, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Aug 17, 2017

What does this do / why do we need it?

Changes a singleSourceCache method's signature from storeVersionMap(versionList []PairedVersion, flush bool) to setVersionMap(versionList []PairedVersion), because all current usages flush and it simplifies the implementations (particularly for the persistent cache, since timestamps/TTLs come into play).

There are also some small related optimizations sprinkled in.

What should your reviewer look out for in this PR?

Reasons to leave the flush parameter as is.

Do you need help or clarification on anything?

Nope.

Which issue(s) does this PR fix?

Related to #431 - Preparation for persistent cache.

@jmank88 jmank88 requested a review from sdboyer as a code owner August 17, 2017 13:04
@sdboyer
Copy link
Member

sdboyer commented Aug 17, 2017

hmmm...i have a vague recollection that i implemented it in the same way this PR changes it to initially, but then switched to the store-and-flush model later. unfortunately, the git blame doesn't have a history on that; even if my recollection is correct, i don't have any hints as to what made me change my mind.

does this make things significantly easier for what you're doing? if so, then i'm OK to bring it in. if not, i'd prefer to keep it as-is, on the assumption that whatever informed my original choice may come back up later, and that we may be happy to have less refactoring, or even rearchitecting, to do then.

@jmank88
Copy link
Collaborator Author

jmank88 commented Aug 17, 2017

does this make things significantly easier for what you're doing?

Yes, both because it's a simpler model to timestamp the whole version set (e.g. don't have to worry about juggling multiple incomplete sets or individual values with differing timestamps), and because it's already implemented.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

ok, good enough 😄

@sdboyer sdboyer merged commit ee6fcfc into golang:master Aug 17, 2017
@jmank88 jmank88 deleted the source_cache branch August 17, 2017 18:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants