wypoon edited a comment on pull request #31871: URL: https://github.com/apache/spark/pull/31871#issuecomment-809805138
LGTM. I commented in the Jira [SPARK-34779](https://issues.apache.org/jira/browse/SPARK-34779) - I don't think that there is a bug as claimed, i.e., I don't think any metrics that are polled are lost, because task metric peaks are sent at task end, and are aggregated. They do not have to come from the executor metric update in a heartbeat. However, this change is an improvement, as it reduces the frequency of removal and insertion of entries in `stageTCMP`. Please update the PR description, as that would become the commit message. I suggest changing the "Why are the changes needed" section to: """ This is an improvement. The current implementation of `ExecutorMetricsPoller` keeps a map, `stageTCMP` of (stageId, stageAttemptId) to (count of running tasks, executor metric peaks). The entry for the stage is removed on task completion if the task count decreases to 0. In the case of an executor with a single core, this leads to unnecessary removal and insertion of entries for a given stage. """ (Note: please correct the spelling of ExecutorMetricsPoller in the previous section and in the title as well.) For the "How was this patch tested" section, you probably want to say that a new unit test is added. -- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
