[JBIDE-21590] Add project cache to handle connection refresh issues

[jcantrill]

cc @fbricon @mlabuda @adietish

[fbricon]

Unit tests?

[scabanovich]

May singleton instance of OpenShiftProjectCache be needed for other clients than only content provider? Then OpenShiftProjectCacheSingleton might be of use as ConnectionsRegistrySingleton is for ConnectionsRegistry.

[scabanovich]

When removing IProjectAdapter, it should be disposed by a dispose() method that would destroy deployment resource mapper and remove listener from connections registry.

[scabanovich]

ServerSettingsViewModel gets from connection projects and builds new DeploymentResourceMapper, could it instead reuse this new cache singleton?

[jcantrill]

@fbricon I was already on it. Added unit tests for the cache.

[jcantrill]

@scabanovich I have not fully thought through the places where one could utilize this cache other then the contentprovider, but one think to note is it is currently impl to only provide UI adapter classes. I would think we would want to keep this out of the ConnectionRegistry. @adietish and I have spoken of a complete resource cache but I think we are still in baby steps before we could fully provide such a solution. I think this is a start towards that goal.

[jcantrill]

@scabanovich I'm not sure I fully grasp your 'dispose' suggestion but welcome examples are additional suggestions.

[scabanovich]

@jcantrill Cache listeners are notified of removed IProjectAdapter and they are already removed from 'cache' map. That is fine. However, DeploymentResourceMapper is still a listener to ConnectionsRegistry, so that it cannot be handled by garbage collector together with all maps that it built. I suggest

public void IProjectAdapter.dispose() {
    mapper.dispose();
}

public void DeploymentResourceMapper.dispose() {
    ConnectionsRegistrySingleton.getInstance().removeListener(connectionListener);
    cache.dispose();
    //clean maps for faster memory cleaning.
}

public void ObservableResourceCache.dispose() {
    //clean maps and collections for faster memory cleaning.
}

[scabanovich]

@jcantrill , I forgot to add that I suggest to call a.dispose() at this point, after listeners are notified that adapter is removed.

[jcantrill]

updated

[jcantrill]

@scabanovich @fbricon updated per comments

[scabanovich]

@jcantrill , I have a comment that may be not quite related to this issue, but it may be not so a big deal to open a new one.
I looked into the way of computing resource property name :

• From kind of resource in ResourcesUIModel

  private static String getProperty(String kind) {
  return StringUtils.pluralize(kind.toLowerCase());
  }

• From tab id in OpenShiftResourcePropertySection.setInput :

  String property = org.apache.commons.lang.StringUtils.right(id, id.length() - id.lastIndexOf(".") - 1);
  

Maybe, there are more places that transform something to property name, I am not aware of them yet. But, imho, even if they are only 2 places and no other will appear, they are too much related to be hidden in the depth of implementation. I suggest a utility that will encapsulate all manipulations with resource property names, (being in an internal package it may have public methods without getting into API), and describe in Java docs in details how it works. Tests for this utility may help to find early if some modification breaks consistency of related names.

[scabanovich]

One more comment, summarizing our discussion in XChat. I focused on method in ResourcesUIModel

public void setBuildConfigs(Collection<IResourceUIModel> buildConfigs) {
    firePropertyChange(PROP_BUILD_CONFIGS, resources.get(ResourceKind.BUILD_CONFIG), resources.put(ResourceKind.BUILD_CONFIG, new ArrayList<>(buildConfigs)));
}

asserting that it will not notify correctly (and other similar setters). That I believe is true. But now I see that actually changes go through methods ResourcesUIModel.add, ResourcesUIModel.remove, ResourcesUIModel.update that have correct notification.
So, if other setters may be ever used, they better be fixed, for instance this way :

public void setBuildConfigs(Collection<IResourceUIModel> buildConfigs) {
    setResourcesForKind(buildConfigs, ResourceKind.BUILD_CONFIG);
}

private void setResourcesForKind(Collection<IResourceUIModel> list, String kind) {
    List<IResourceUIModel> oldList = resources.get(kind);
    List<IResourceUIModel> newList = new ArrayList<IResourceUIModel>(list);
    resources.put(kind, newList);
    firePropertyChange(getProperty(kind), oldList, newList);
}

and with use of private setResourcesForKind all other setters will remain one-liners.

[jcantrill]

@scabanovich please open a separate issue regarding your observations. I will merge this PR as is unless there are objections from @fbricon or @adietish