add spec cache #1066
add spec cache #1066
Conversation
| } | ||
|
|
||
| @Override | ||
| public void put(String imageName, ConnectorSpecification spec) { |
There was a problem hiding this comment.
opted not to use the LoadingCache that guava has (where it does fetch-if-absent behavior). mostly, i didn't like the error handling.
this whole iface exists though because i want to be able to change my impl easily if i decide i messed up here.
cgardens
requested review from
sherifnada,
michel-tricot and
ChristopheDuong
There was a problem hiding this comment.
Nice.
My main concern is that caching is leaking outside the scheduler handler. Why don't we encapsulate it inside the get spec method of the scheduler handler entirely? This means we need to restart the dev server if we ever hotfix an image, but this can be handled internally by using the SHA of the image we use or something like that (or for now add a time-based expiry to the cache).
| .withDestinationDefinitionId(currentDestination.getDestinationDefinitionId()) | ||
| .withDockerImageTag(destinationDefinitionUpdate.getDockerImageTag()) | ||
| .withDockerRepository(currentDestination.getDockerRepository()) | ||
| .withName(currentDestination.getName()) | ||
| .withDocumentationUrl(currentDestination.getDocumentationUrl()); | ||
|
|
||
| configRepository.writeStandardDestinationDefinition(newDestination); | ||
| // we want to re-fetch the spec for updated definitions. | ||
| specCache.evict(destinationDefinitionUpdate.getDockerImageTag()); |
There was a problem hiding this comment.
should this be the whole image name? right now it's just the tag
could you say more about what you mean by this? what you're saying may be clarified with this: keep in mind that the handlers are all ephemeral. there is no guarantee that they survive from one api call to another or that the same handlers are used to field api calls from the same caller. in order for the cache to be used across all handlers it has to be up in ServerApp.
so what you're proposing is that the handler shells out to docker and tries to look up the sha has of an image to see if it has changed? i'm reticent to add a docker dependency on the server. do you think it's worth it?
i feel pretty strongly that i don't want to do this now. adding time-based anything to this adds a really obnoxious set of cases you need to think about when you're debugging these things. we should only use it if it serves an important use case. in this case, we are adding very few entries to the cache and other than changing the tags we never want to change the entries (except in a well known development use case for which there is a workaround). |
|
I believe there must a way to hide the caching logic so that it doesn't spread in the codebase like it does right now. Also It is not completely clear to me why we need an eviction logic since spec is supposed to be immutable and tied to (imagename,version) Let's take sometime to discuss it. |
|
Sounds good!
This is contributor DX. If the version of the connector to you are using is "dev" as you iterate on it, you want to be able to overwrite the existing one. Was one of the things that zazmic got stuck on. |
What
How