You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/09/11 11:36:17 UTC
[GitHub] [beam] scwhittle opened a new pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
scwhittle opened a new pull request #12825:
URL: https://github.com/apache/beam/pull/12825
…he original System.err
Currently such errors are logged to System.err which is a PrintStream that
publishes to the handler. This is perhaps unlikely to work if earlier publishing
failed and additionally removes a potential deadlock between the PrintStream
object sychronization and the Handler object synchronization. This was attempted
to be fixed earlier by dissallowing the PrintStream object to be synchronized
when calling into the handler. However this is possible to be triggered by
external synchronization on the PrintStream, such as that performed by
Throwable.printStackTrace. Changing the PrintStream to use separate synchronization
for buffering works in most cases but not for cases where the stream is externally
synchronized.
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691258077
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-698861128
I have been busy with other higher-priority work but am planning on
finishing it up next week
On Fri, Sep 25, 2020 at 1:55 AM Ahmet Altay <no...@github.com>
wrote:
> What is the next step for this PR?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/12825#issuecomment-698645640>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBZZTD2IMUNHYRAG6EBX53SHPL7XANCNFSM4RHI5ZUQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691258077
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691048364
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-698645640
What is the next step for this PR?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-692175069
@kennknowles `system.err` is always intended to log at ERROR level. Are you saying that logging is being misconfigured somehow and logs for other severities are being sent to `system.err`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-702944699
We can leave as is since the migration to using the Java SDK harness is upcoming and the Java SDK harness doesn't override system.out/system.err due to the long list of issues we have hit with this override to capture system.out/system.err logs in Dataflow.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-692173705
@scwhittle
I see how the deadlock occurs but why introduce the synchronized(buffer) if we are swapping out the error manager?
Also, wouldn't we solve the locking problem if we always held the same "flush" lock when interacting with the error manager?
We could do this by sharing a single lock object across the error manager, print stream, ...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-692268316
Had a chat and we confirmed that the worker logging setup already handles my concern appropriately. Please disregard all comments.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691048364
@lukecwik Luke can you review as you have context from last time?
Changing to synchronizing on buffer instead of the PrintStream changes the precondition to just enforce the invariant on when publish is called within our custom PrintStream. That means that the original deadlock can still occur if this happens:
T1: synchronizes on System.err (Throwable.printStackTrace for example), publishes to handler
T2: synchronized within handler, tries to report error to System.err using Throwable.printStackTrace which synchronizes on the PrintStream
I removed that case by using the custom ErrorManager to print to the original stderr stream, an alternative would be to change the ErrorManager still use our custom PrintStream but remove use of Throwable.printStackTrace so as not to sychronize on the PrintStream.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-698645640
What is the next step for this PR?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-698861128
I have been busy with other higher-priority work but am planning on
finishing it up next week
On Fri, Sep 25, 2020 at 1:55 AM Ahmet Altay <no...@github.com>
wrote:
> What is the next step for this PR?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/12825#issuecomment-698645640>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBZZTD2IMUNHYRAG6EBX53SHPL7XANCNFSM4RHI5ZUQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] lukecwik merged pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #12825:
URL: https://github.com/apache/beam/pull/12825
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-702981230
> Please work with @robinyqiu to get this cherry picked into the 2.25 release branch.
Should https://issues.apache.org/jira/browse/BEAM-9399 be marked as a release blocker?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-702943945
Run Java PreCommit
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691048364
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-692275145
@lukecwik I think to have a single lock, we would need to use the PrintStream lock itself, as that is what is synchronized on by Throwable.printStackTrace and which can be synchronize outside and inside other synchronization blocks if we choose another lock to guard the buffer, ErrorManager, handler.
However using the PrintStream as a lock within the DataflowWorkerLoggingHandler seemed perhaps error prone as that extends a Handler which might have it's own synchronized methods (on itself not the PrintStream) or callers that synchronize on the Handler (and not on the PrintStream).
Additionally it seemed a bit gross since there is not always a PrintStream for a handler, though we could just allow injecting a lock Object into the handler to hide that.
If you think that approach is still preferrable, I can put that together.
Regarding on why to synchronize on buffer, that allows us to keep the preconditioncheck to sanity check our implementation. But that seems overkill, so I will just remove it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691258077
I believe the trouble here is that it mixes all log levels on stderr, while the log reporting in the UI reports all stderr lines at the same severity. I cannot recall if they are all INFO or all WARN in the UI, but either way it is incorrect and confusing. So we need the corresponding change to make sure that doesn't happen. Just drive by comment because I've encountered this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-692236055
@lukecwik that is how I feel about System.err as well. Java disagrees and writes all logs to stderr unless you disconnect that. IIRC I discovered through some other work that it is not disconnected here.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-691867771
I am only changing how errors from within the DataflowWorkerLoggingHandler itself are reported, for example an error publishing to stackdriver. I agree that logging to stderr is difficult to view in the UI, but that seems separate from the deadlock we originally fixed (the Jira has more details) and the erroneous precondition introduced when that was added. Kenn, does the limited scope of this change address your concerns? I am not changing it to use the original System.err generally
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] scwhittle commented on pull request #12825: [BEAM-9399] Change DataflowWorkerLoggingHandler to report errors to t…
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #12825:
URL: https://github.com/apache/beam/pull/12825#issuecomment-699868195
@lukecwik PTAL, see above for why I think having a single lock would be hard/possibly error-prone. Let me know if you think it's worth pursuing that.
----------------------------------------------------------------
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:
users@infra.apache.org