[GitHub] rdhabalia commented on issue #507: Add zk-stats instrumentation to get zk-client stats

[gi...@git.apache.org]

rdhabalia commented on issue #507: Add zk-stats instrumentation to get zk-client stats
URL: https://github.com/apache/incubator-pulsar/pull/507#issuecomment-314275283
 
 
   > Can't the latency measurement be done in the ZK client wrapper?
   
   Latency is measured against [ZK-Client](https://github.com/rdhabalia/pulsar/blob/ce73e74e2702b32cbd9a553949017033b901e1d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/zookeeper/aspectj/ClientCnxnAspect.java#L64) `org.apache.zookeeper.ClientCnxn` only. So, you mean `ClientCnxnAspect` should not be part of pulsar-broker and part of other module?
   
   > Though I don't completely understand if the AOT part is needed if we are going to wrap the ZooKeeper client class.
   
   Not, completely understand your comment. But in this PR we are loading [AOT-agent at runtime](https://github.com/rdhabalia/pulsar/blob/ce73e74e2702b32cbd9a553949017033b901e1d2/pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandaloneStarter.java?utf8=%E2%9C%93#L160) because of that we don't have to define agent in jvm-args and we can test aspect with unit-test. 
   In previous ZK-server-aspect PR, we don't have any unit-test case because we can't weave aspectj-  advice in unit-test so, hard to test as well. So, do you think we should load aspectj-agent at runtime rather passing as jvm-args in zk-server AOT?
 
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services