CDAP-5277 added caching of plugin instantiators #5528
Conversation
|
build running : http://builds.cask.co/browse/CDAP-DUT3939-1 |
|
|
||
| @Override | ||
| public void onRemoval(RemovalNotification<ArtifactDescriptor, Instantiators> notification) { | ||
| Closeables.closeQuietly(notification.getValue()); |
There was a problem hiding this comment.
is anything ever removed from the cache? Couldn't see where that would happen
There was a problem hiding this comment.
have updated the cache policy to include expireAfterAccess for 1hr
There was a problem hiding this comment.
I think it has to be by size as well.
There was a problem hiding this comment.
sorry, just seeing this. the cache key is parent artifact descriptor, so the size of the cache (number of elements) would be less, though the size of the value could be bigger as more plugin methods are invoked. maybe we can set maximumWeight with a custom weigher.
gsps1
force-pushed
the
feature/caching-plugin-instantiator
branch
2 times, most recently
from
f5f1c40
to
83315a9
Compare
|
thanks for the review, have addressed the comments @albertshau |
| @@ -473,6 +472,12 @@ public void callArtifactPluginMethod(HttpRequest request, HttpResponder responde | |||
| "Received empty request body."); | |||
| } | |||
|
|
|||
| // should not happen | |||
| if (!pluginService.isRunning()) { | |||
There was a problem hiding this comment.
if we're worried about this, the logic should be in PluginService rather than in the caller
|
few more comments |
gsps1
force-pushed
the
feature/caching-plugin-instantiator
branch
from
fe025a0
to
ab0cdb1
Compare
|
lgtm |
gsps1
force-pushed
the
feature/caching-plugin-instantiator
branch
from
ab0cdb1
to
61e7d8d
Compare
|
rebased, merging. |
JIRA: https://issues.cask.co/browse/CDAP-5277