[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2504 @revans2 Great news! Looking forward to new news about that. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2504 @HeartSaVioR not a problem. I often forget that others are not able to read my thoughts and I sometimes forget to mention the context. The code was not really a prototype. We intend it to be production ready, but before pushing anything large like this in we want to harden it with some stress tests. That was something that we had not done yet. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2504 @revans2 Thanks a lot for sharing current status. Actually I have been aware of the patch going through @abellina forked repo, so I expect HBase plugin and UI patch would be adopted internally, and I also expect most of metrics would be integrated with new system. I thought only metrics V2 is a blocker for your team to bring such features to Apache side, and thatâs why I really would want to integrate metrics V2 sooner. I was not aware that they were prototypes instead of production ready, hence also thought it can be included to Storm 2.x sooner (2.0.0 ideally). If this patch itself is for baseline of other features like elasticity (its plan is also not transparent and I am not aware of that) I think the patch is welcome to get reviewed independently and merged in. I wasnât just not clear what benefits the patch will provide. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2504 @HeartSaVioR sorry about that I think we probably were not clear enough on this, what is covered and what is not currently covered by this pull request. Reading through the description of STORM-2156 you are 100% right that this does not cover everything there. I will file a new subtask for STORM-2156 and we will update this pull request to be under it. I am sorry about the confusion. Thanks for calling us out on this. For those who care here is some more history about this patch: Originally this work was based off of the metrics v2 patch and was done by @abellina and @lavindev as an intern here. But it ended up being a very large patch and it looked like it would take a very long time to go in. We would have to wait for metrics v2 to go in, then get this reviewed and in along with the glue code that is not here, and then have both of them ported to master. To try and speed it up I asked @agresch to take out just the rocksdb metrics storage piece and a small number of metrics that don't require the v2 patch, put it on master, make sure it is solid and see if we can get that in by itself. I thought this would be great because it would at least provide the minimum metrics needed to start looking at elasticity. The prototype that @lavindev and @abellina includes all of the integration with metrics v2, the UI updated to show both sets of metrics side by side so we could judge how close they were to each other, a real time metrics graphing proof-of-concept on the UI, and a plugin to store the metrics in HBase instead of rocksdb. So we have code to cover all of STORM-2156 and more. We are just trying to optimize how quickly we can get it in so we can hopefully do a 2.x release sometime this quarter. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2504 I left the questions because this PR looks like addressing only part of STORM-2156. The summary of issue STORM-2156 is: ``` Add RocksDB instance in Nimbus and write heartbeat based metrics to it ``` and description is: ``` There should be a RocksDB instance in Nimbus where we write metrics from the heartbeats. This should allow us to replace storage for the statistics we see in the UI and expand the abilities of UIs to allow for time series charting. Eventually this data will likely come via thrift to Nimbus as the overall metric system is overhauled. ``` (Even STORM-2156 should have follow up issue for representing RocksDB metrics to UI.) In order to replace the metrics data in ZK heartbeat, it should be mandatory to address worker metrics (to supervisor) to nimbus. I don't mind it would be based on STORM-2693 which transfers worker metrics into Nimbus (addressing Metrics V1), or it would be following up patch for STORM-2153 (Metrics V2) being implementation of metrics collector which communicates to Nimbus. (For latter we should migrate most of built-in metrics to Metrics V2 so that it can be available.) Actually it only addresses metrics transfer from supervisor to nimbus which doesn't bring origin intention and benefits of issue. I guess you have follow up issues or even patches but they're opaque for me now so I don't see much benefit of this. Please also file follow up issues and group issues together (design doc explaining overall plan would be great) so that we can imagine what will be changed and how the changes improve Storm. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2504 Before looking into the patch in detail, I'd like to see which functionalities the patch would want to bring. 1. As far as I read from MetricStore, it looks like providing an aggregated point from time-range query. Do I understand correctly? Because what I've expected is like a time-series one, and it would replace metrics consumers and eventually provide time-series representation. I expect it can still replace metrics consumers for the store side btw. 2. I guess I know the answer (at least partially) and eventual following-up patch would be storing metrics into external storage (like HBase), but just to double check: how it will behave when leader Nimbus goes down and one of standby Nimbus promotes to leader? Will metrics stored previously be unavailable? If Nimbus gets leadership again, how Nimbus shows the gap while it didn't receive the metrics? (especially aggregated values) Do we want to apply interpolation, or just treat it as no metrics (hence 0 for sum and None for avg)? ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user agresch commented on the issue: https://github.com/apache/storm/pull/2504 Further metrics can be added to the database as desired. The intent of this patch was to get the database functional. Given the patch size and ongoing metrics work, I would suggest adding more metrics as a follow on JIRA. ---
[GitHub] storm issue #2504: STORM-2156: store metrics into RocksDB
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2504 I think this patch (or following up patch, at least) should address workers' metrics as well. Is it just waiting for Metrics V2? (#2203) Or to avoid backward incompatibility between old workers? ---
