Skip to content

[BEAM-9242] No longer reporting Lulls as errors in the worker.#10742

Merged
pabloem merged 1 commit intomasterfrom
pabloem-patch-1
Feb 3, 2020
Merged

[BEAM-9242] No longer reporting Lulls as errors in the worker.#10742
pabloem merged 1 commit intomasterfrom
pabloem-patch-1

Conversation

@pabloem
Copy link
Member

@pabloem pabloem commented Feb 1, 2020

These were misleading users to think their pipeline was having actual errors when it was in fact performing fine.

Furthermore, this reporting had been added to ensure users would be able to notice these logs. With the new Dataflow Job Details page, there is a Worker Logs section that shows logs from all workers, and will expose these logs as warnings rather than errors.

r: @boyuanzz


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Copy link
Contributor

@boyuanzz boyuanzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pablo!

* lull.
*/
@Override
public void reportLull(Thread trackedThread, long millis) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we log these info as warning for debugging purpose?

@boyuanzz
Copy link
Contributor

boyuanzz commented Feb 1, 2020

Also could you please add a JIRA issue to track this improvement?

@pabloem pabloem changed the title No longer reporting Lulls as errors in the worker. [BEAM-9242] No longer reporting Lulls as errors in the worker. Feb 1, 2020
@pabloem
Copy link
Member Author

pabloem commented Feb 1, 2020

This removes the streaming subclass implementation that logs as a Dataflow system error. The Dataflow implementation already logs this as a warning log coming from the worker:

@Override
public void reportLull(Thread trackedThread, long millis) {
// If we're not logging warnings, nothing to report.
if (!LOG.isWarnEnabled()) {
return;
}
// Since the lull reporting executes in the sampler thread, it won't automatically inherit the
// context of the current step. To ensure things are logged correctly, we get the currently
// registered DataflowWorkerLoggingHandler and log directly in the desired context.
LogRecord logRecord =
new LogRecord(Level.WARNING, getLullMessage(trackedThread, Duration.millis(millis)));
logRecord.setLoggerName(DataflowOperationContext.LOG.getName());
// Publish directly in the context of this specific ExecutionState.
DataflowWorkerLoggingInitializer.getLoggingHandler().publish(this, logRecord);
}

Also filed BEAM-9242 : )

Copy link
Contributor

@boyuanzz boyuanzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pablo! Just curious are we going to have the similar change in batch, or we are just ok now?

@pabloem pabloem merged commit a555567 into master Feb 3, 2020
@pabloem
Copy link
Member Author

pabloem commented Feb 3, 2020

thanks @boyuanzz - Batch does not have the speciall handling that streaming has. Now Batch/Streaming work the same.

@pabloem pabloem deleted the pabloem-patch-1 branch February 3, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants