Skip to content

Conversation

wujimin
Copy link
Contributor

@wujimin wujimin commented Oct 28, 2017

edge use new InstanceCacheManager, normal consumer use old InstanceCacheManager
if there is no problem, then old InstanceCacheManager will be deleted.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 86.444% when pulling 03af3ad on wujimin:edge-lb-new-instance-cache-manager into b11df2b on ServiceComb:master.

@Override
public ClassLoader create(String microserviceName, String version) {
public ClassLoader create(String appId, String microserviceName, String version) {
return Thread.currentThread().getContextClassLoader();
Copy link
Member

Choose a reason for hiding this comment

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

If we just return the TCC, maybe we need to update the method name for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this just a Default factory, another PrivateXXXFactory will do a real create

@@ -0,0 +1,3 @@
cse-config-order: -500
Copy link
Member

Choose a reason for hiding this comment

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

Please add the license header here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed, no need to add license here.


String microserviceVersionFactoryClass = serviceRegistryConfig.getMicroserviceVersionFactory();
if (microserviceVersionFactoryClass == null) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

It's better to throw exception here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this version, we did not remove old cacheManager, and new cacheManager only used by edge.
so microserviceVersionFactoryClass is null, means not work with edge, will use old cacheManager, can not throw exception

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment to the code for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@wujimin wujimin force-pushed the edge-lb-new-instance-cache-manager branch from 03af3ad to 2f4bbe7 Compare November 1, 2017 02:56
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.472% when pulling 2f4bbe7 on wujimin:edge-lb-new-instance-cache-manager into f1b0cbe on ServiceComb:master.

@WillemJiang WillemJiang merged commit 4642127 into apache:master Nov 3, 2017
@wujimin wujimin deleted the edge-lb-new-instance-cache-manager branch November 14, 2017 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants