[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Cool, thanks @Randgalt, let me know when we decided the released schedule. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt we haven't started the unifying progress yet, since we were blocked by this feature, it would be better to publicize it on Curator's wiki when we're actually using it inside FB, I'll let you know, and feel free to publicize this on Curator's wiki then :) Unifying the ZooKeeper Java client is our Q4 Goal, and it takes time to ask/help the internal teams to switch to Curator, so it would be great if we can have a release at the end of Oct or beginning of Nov. Inside Facebook, we're still using ZK 3.4, so can we have a release in Curator 2.xxx which include this change? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 @lvfangmin > we're planning to unify the Zookeeper java client to be Curator inside Facebook Wow - cool! Can I publicize that on Curator's wiki? > may I ask what's the next scheduled release? And will this patch be in it? Personally, I'm waiting on ZOOKEEPER-2169 and ZOOKEEPER-1525 and was planning on doing a build when those are ready. Supposedly it will be in ZK 3.6. How long can you wait? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt @cammckenzie Thanks for reviewing the code, we're planning to unify the Zookeeper java client to be Curator inside Facebook, so it's really important for us to track the client side metrics. Since we're depending on this patch to unify the java client, may I ask what's the next scheduled release? And will this patch be in it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Yes, feel free to join remote, we'll post the ways to join online soon. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 Sounds like they'll have a remote option - I'm remote as well --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/165 Thanks @lvfangmin, unfortunately I'm somewhat geographically challenged living on the other side of the world. Sounds like fun though. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt @cammckenzie, Facebook is going to hosted a Zookeeper Meetup, there will be tech talks, swags, drinks and snacks, please join us if you're interested in: https://www.facebook.com/events/1228722650504268/. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Hi @Randgalt, I've opened another PR for 3.0 branch, could you please take a look? Let me know if there is any issue. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Thanks @Randgalt, I've tried to merge this, there is not much conflict, I'll send a separate PR based on CURATOR-3.0 today. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 I think we're almost there. Have you tried merging this with the CURATOR-3.0 branch yet? Because this is a biggish change it would be good to have a separate PR based off of CURATOR-3.0 to make sure there are no conflicts there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Hi @Randgalt, in case you're not aware, I've updated the code base are you opinion, let me know if you have other comments, thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 I'll try to get to this soon. It's a big-ish PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 kindly ping. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Please take a look when you guys have time :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt I've updated the diff based what we discussed, please take a look when you have time. And have a great weekends! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Thanks @Randgalt, I'll update the PR based on this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 Yes - that should work. Create a new set of APIs but still support the old API inside of the new one and mark the old one as deprecated. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt thanks for giving suggestions, there is another option: we create a new tracer interface, like AdvancedTracerDriver which will expose more metrics like we're doing in this PR. What's your idea? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 I haven't looked at the PR in depth. But, having this PR be backward compatible should be explored. Possibly by emulating the old SPI/API. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @Randgalt with new methods in the existing classes will also changes the interface the users are using, it has the same side effect as this one, right? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/165 I'm concerned that this change will force a major version bump from us. Can't this be done with the existing classes and a few new methods? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 @cammckenzie Thanks again for reviewing the code, I think the millis is good enough for tracing. I'll wait @Randgalt's review :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/165 I've had a bit more of a look and I think that the changes are OK. As you mentioned, the changes are only to internal classes. There is a loss of granularity (The TimeTrace was using nanos while your new implementation is using millis), but I doubt that this is an issue in practice. So, I'm happy with the changes. @Randgalt is away at the moment, but I think that he should review it when he returns. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 ping. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/165 Thanks @cammckenzie for review, the TimerTrace is actually an internal helper class, it shouldn't have any backwards compatible problem. It is the TracerDriver that may have backwards compatible issue, but more strictly it's not a backwards compatible issue, it's that the users who are using TracerDriver have to change their implementation based on the new interface. @Randgalt can you help review on it, it's a feature required internally and we want to have it as soon as possible, thanks :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/165 Thanks for the PR, The content looks OK, but I think that the interface changes need to be made backwards compatible. Can we change the OperationTrace to extend the existing TimerTrace? @Randgalt I'm not really familiar with this code, so it could probably use another set of eyes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
