Add peak memory distribution among tasks of each stage by viczhang861 · Pull Request #13379 · prestodb/presto

viczhang861 · 2019-09-11T04:41:26Z

Resolves #13327

== RELEASE NOTES ==

General Changes
* Add peak total memory distribution among tasks of each stage

aweisberg

One small nit about TaskDistribution constructor. Otherwise it makes sense to me.

aweisberg · 2019-09-11T22:38:34Z

presto-spi/src/main/java/com/facebook/presto/spi/eventlistener/TaskDistribution.java

Just curious, why reduce precision?

For current purpose of using this class for either memory bytes or cpu time, float precision is enough.

Changed it back double to avoid confusion

aweisberg · 2019-09-11T22:40:04Z

presto-main/src/main/java/com/facebook/presto/event/QueryMonitor.java

Constructing a task distribution is very verbose and a lot of this stuff comes from a DistributionSnapshot, can you update the TaskDistributionConstructor to reduce some of this duplication by working on the DistributionSnapshot directly?

This is intended to support serialization into json string. There is downstream dependency on structure of this class, e.g. visualization of distribution.

mbasmanova

Add peak total memory of a task to TaskStats

Some questions.
I'd update commit message to Add peak total memory to TaskStats

mbasmanova · 2019-09-13T17:20:58Z

presto-main/src/main/java/com/facebook/presto/operator/TaskContext.java

cumulativeUserMemory is stored as double; peakTotalMemoryInBytes seems similar, but it stored as long; why is the discrepancy?

peakTotalMemoryInBytes name is not consistent with cumulativeUserMemory; shouldn't it be peakTotalMemory?

cumulativeUserMemory uses double as its unit is Bytes * Time, this explains the difference of names

@viczhang861 Vic, thanks for explaining. It makes sense now.

mbasmanova · 2019-09-13T17:22:51Z

presto-main/src/main/java/com/facebook/presto/operator/TaskStats.java

Looking at cumulativeUserMemory, I'd expect

@JsonProperty("peakTotalMemory") double peakTotalMemory,

Or peakTotalMemoryReservation and use DataSize as type? No strong opinion here.

mbasmanova

Rename StageCpuDistribution to TaskDistribution

mbasmanova · 2019-09-13T17:26:02Z

presto-main/src/main/java/com/facebook/presto/event/QueryMonitor.java

I'm wondering if it would be useful to keep Stage in the name. The data is the distribution of task resource usage within a stage, right? StageResourceDistribution maybe?

Updated to ResourceDistribution to avoid confusion

mbasmanova · 2019-09-13T17:27:57Z

presto-spi/src/main/java/com/facebook/presto/spi/eventlistener/TaskDistribution.java

This is unrelated change and therefore should go into separate commit. My preference would be to keep double though. Primarily to avoid having other readers wonder why float.

mbasmanova

Add peak memory distribution among tasks of each stage

Looks good.

mbasmanova · 2019-09-13T17:28:54Z

presto-main/src/main/java/com/facebook/presto/event/QueryMonitor.java

nit: perhaps, computeResourceDistributions or computeCpuAndMemoryDistributions

oerling · 2019-09-13T17:41:03Z

This is all fine by me. I tested it and the data looks right. From: Maria Basmanova <notifications@github.com> Sent: Friday, September 13, 2019 10:31 AM To: prestodb/presto <presto@noreply.github.com> Cc: oerling <erling@xs4all.nl>; Review requested <review_requested@noreply.github.com> Subject: Re: [prestodb/presto] Add peak memory distribution among tasks of each stage (#13379) @mbasmanova <https://github.com/mbasmanova> requested review from @prestodb/aria on: #13379 <#13379> Add peak memory distribution among tasks of each stage. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#13379?email_source=notifications&email_token=AKPPPT6ZQAE6R227Y7DZ3ODQJPE4LA5CNFSM4IVPFQH2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTTYTCNA#event-2633052468> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AKPPPTZOKG5KYYWQVBMVKSDQJPE4LANCNFSM4IVPFQHQ> .

mbasmanova

@viczhang861 Thanks.

wenleix

LGTM.

wenleix · 2019-09-18T05:08:20Z

presto-main/src/main/java/com/facebook/presto/operator/TaskStats.java

Or peakTotalMemoryReservation and use DataSize as type? No strong opinion here.

wenleix · 2019-09-18T05:10:15Z

presto-spi/src/main/java/com/facebook/presto/spi/eventlistener/QueryStatistics.java

What about taskPeakMemoryDistribution? No strong opinion since peakMemoryDistribution is parallel to cpuTimeDistribution

This class can be used for either memory or cpu distribution of tasks in a stage.

facebook-github-bot added the CLA Signed label Sep 11, 2019

viczhang861 force-pushed the write-task branch 3 times, most recently from 83e8cf1 to f85def4 Compare September 11, 2019 15:15

viczhang861 requested review from mbasmanova and wenleix September 11, 2019 17:39

viczhang861 changed the title ~~[WIP]Add peak memory distribution among tasks of each stage~~ Add peak memory distribution among tasks of each stage Sep 11, 2019

aweisberg self-requested a review September 11, 2019 18:37

aweisberg suggested changes Sep 11, 2019

View reviewed changes

mbasmanova reviewed Sep 13, 2019

View reviewed changes

mbasmanova requested a review from a team September 13, 2019 17:30

mbasmanova self-assigned this Sep 13, 2019

viczhang861 force-pushed the write-task branch from f85def4 to 50f82b9 Compare September 13, 2019 19:51

viczhang861 assigned aweisberg and mbasmanova and unassigned mbasmanova Sep 13, 2019

viczhang861 requested a review from aweisberg September 13, 2019 19:55

mbasmanova approved these changes Sep 13, 2019

View reviewed changes

aweisberg approved these changes Sep 16, 2019

View reviewed changes

wenleix approved these changes Sep 18, 2019

View reviewed changes

viczhang861 force-pushed the write-task branch from 50f82b9 to dab55fb Compare October 22, 2019 20:05

viczhang861 added 3 commits October 23, 2019 17:25

Add peak total memory to TaskStats

11669d2

Rename StageCpuDistribution to ResourceDistribution

9b71f97

This class can be used for either memory or cpu distribution of tasks in a stage.

Add peak memory distribution among tasks of each stage

dba96e9

viczhang861 force-pushed the write-task branch from dab55fb to dba96e9 Compare October 23, 2019 21:41

arhimondr merged commit 88416e5 into prestodb:master Oct 24, 2019

caithagoras mentioned this pull request Nov 11, 2019

Add release notes for 0.229 caithagoras/presto#2

Closed

14 tasks

This was referenced Nov 11, 2019

Add release notes for 0.229 caithagoras/presto#3

Closed

Add release notes for 0.229 caithagoras/presto#4

Closed

Add release notes for 0.229 caithagoras/presto#5

Closed

Add release notes for 0.229 #13686

Merged

viczhang861 deleted the write-task branch May 5, 2021 06:02

Conversation

viczhang861 commented Sep 11, 2019 • edited by wenleix Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viczhang861 Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oerling commented Sep 13, 2019 via email

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

viczhang861 commented Sep 11, 2019 •

edited by wenleix

Loading

viczhang861 Sep 11, 2019 •

edited

Loading