[pulsar-discovery] Replace MetadataStore with ZooKeeper in discoveryservice #9967
Conversation
rdhabalia
requested review from
merlimat,
sijie,
jiazhai,
codelipenghui and
nkurihar
|
|
||
| // DiscoveryServiceServlet gets initialized by a server and this map will help to retrieve ZK while mocking | ||
| // DiscoveryServiceServlet | ||
| private static final Map<String, MetadataStoreExtended> metadataStoreInstanceCache = Maps.newConcurrentMap(); |
There was a problem hiding this comment.
it looks like we are not clearing this map at the end of the test.
we will leak MetadataStoreExtended instances
There was a problem hiding this comment.
It's not leaking. static map is intentional because of the DiscoveryServiceServlet which gets initialized by Webserver and we need to inject MetadataStore into dynamically initialized servelet. so, DiscoveryServiceServletTest extends the discovery-servlet class and access metadata store from this static map.
It's not leaking because everytime when test starts it puts the metadatastore into the map and then it closes the metadata store at the end of test so, it will not leak.
So, static map is the way to access metadata-store from dynamically initialized servlet which we can't be mocked or controlled.
There was a problem hiding this comment.
I am fine with having the static map. it is a good idea.
I would only add a
@AlfterClass
public void clearMetadataStoreInstanceCache() {
metadataStoreInstanceCache.clear();
}
if we do not clear the map then we are keeping a reference to the (closed) MetadataStore into a static variable.
| @@ -56,6 +56,9 @@ | |||
| public class DiscoveryServiceWebTest extends ProducerConsumerBase { | |||
|
|
|||
| private final Client client = ClientBuilder.newClient(new ClientConfig().register(LoggingFeature.class)); | |||
| // DiscoveryServiceServlet gets initialized by a server and this map will help to retrieve ZK while mocking | |||
| // DiscoveryServiceServlet | |||
| private static final Map<String, MetadataStoreExtended> metadataStoreInstanceCache = Maps.newConcurrentMap(); | |||
There was a problem hiding this comment.
it looks like we are not clearing this map at the end of the test.
we will leak MetadataStoreExtended instances
…service fix test fix test fix test
Motivation
one of the Implementations of #9646 . Remove direct ZooKeeper dependency from discovery-service and replace with metadata-store. Rename
ZookeeperCacheLoadertoMetadataStoreCacheLoaderand use MetadataStore access zk.