[GitHub] [incubator-uniffle] jerqi commented on pull request #387: [WIP][ISSUE-333] Expose metrics of TOP10 app bytes in one shuffle server

[GitBox <gi...@apache.org>]

jerqi commented on PR #387:
URL: https://github.com/apache/incubator-uniffle/pull/387#issuecomment-1336809898

   > The modification direction looks good to me.
   > 
   > But the metrics looks incomplete. I think we should add the metrics, including the whole storage(memory&localfile&hdfs) occupied ratio and bytes for top10 single app, instead of only memory. But should we need the original metric output style, I have not a good idea yet. The original metric output style looks inproper for this scenario.
   > 
   > ```yaml
   > // original metric output style
   > {
   > "metrics": [
   > {
   > "name": "xxx",
   > "labelNames": [],
   > "labelValues": [],
   > "value": 41332,
   > "timestampMs": null
   > }
   > ]
   > }
   > ```
   > 
   > And the top10 sorting, I think It only should be triggered by the scheduler thread to update, instead of updating every time on `updateCachedBlockIds`. Maybe this will cause performance regression.
   > 
   > Do you have other ideas on this? @jerqi @kaijchen .
   
   I think application level memory size is enough. Because the pressure mainly come from the memory which need to be flushed instead of data size of LOCALFILE or HDFS.


-- 
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: issues-unsubscribe@uniffle.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org