asafm commented on PR #17041: URL: https://github.com/apache/pulsar/pull/17041#issuecomment-1238253019
> > > for 1, if I change protected AbstractMetadataStore() to protected AbstractMetadataStore(String metadataStoreName), > > > > > > Maybe I misunderstand. You have 5 classes that extend `AbstractMetadataStore`, which are not abstract, right? If I understand correctly, you only need to modify them, no? > > not only them, `Factory` and related tests also need to fix. Toooooo many tests referenced it Ok, I took a closer look at the code, and you are correct. You fix the factory, and give the proper names (apparently instances of metadata stores are created in numerous places, tests excluded), but you end with at least 67 places in the tests that instantiate metadata using the factory and those needs to be fixed. But, it got me thinking: What is the purpose of this PR? We want to know the operations count and latency on the metadata stores. How do we want them broken down? * Purpose? Pulsar Metdata Store, Pulsar Configuration Store, BK metadata store, etc... * Type? ZK, etcD? * component: metadata store, global configuration store. What will help the operator understand and pinpoint? In the current implementation in this PR, you don't know which metadata store it refers to: Is the metadata store or configuration store? Is it the metadata store used by the BK client, or something else? What you see is "metadata-store-0" which tells you nothing. If it reveals nothing, maybe we don't need it at all - the name that is. So that's why I'm asking the questions above. I think those questions are important. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
