Conversation
|
R: @pabloem |
|
Run Java PreCommit |
|
Run Java_Examples_Dataflow PreCommit |
|
Run JavaPortabilityApi PreCommit |
pabloem
left a comment
pabloem
left a comment
There was a problem hiding this comment.
This generally looks fine to me. I left a couple comments : )
| // The MapTask instruction is ordered by dependencies, such that the first element is | ||
| // always going to be the shuffle task. | ||
| String stepName = computationState.getMapTask().getInstructions().get(0).getName(); | ||
| hotKeyLogger.logHotKeyDetection(stepName, hotKeyAge); |
There was a problem hiding this comment.
Is the stepName the correct step name that we want? (e.g. not s2, nor GroupByKey/ReadFromShuffle, but rather, the user-known GroupByKey step?)
There was a problem hiding this comment.
Yep! Just tested it, looks good! Also the comment for the "getName" specifies that it is the user-provided name.
| if (isThrottled()) { | ||
| return; | ||
| } | ||
| LOG.warn(getHotKeyMessage(userStepName, TimeUtil.toCloudDuration(hotKeyAge))); |
There was a problem hiding this comment.
Is warning the right log level for this? It may be "normal", and acceptable to have a hot key - and when users see a warning they'll think something bad is happening. What do you think?
The same thing has happened with lull logging, where users think something's wrong - though that's not the case.
There was a problem hiding this comment.
Yes, the detection from the service is explicitly only when the hot key is causing slowness/ limited parallelism.
2 participants
Adds HotKeyDetection to the StreamingDataflowWorker. Also moves the existing HotKeyDetection into a shared class between Batch and Streaming.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.