[BEAM-11474] Track transform processing thread in Java SDK harness and set log entry field #13533
Conversation
transform id field.
y1chi
changed the title
|
R: @boyuanzz |
y1chi
changed the title
There was a problem hiding this comment.
Currently you have recorded the threadId-transformId mapping on
- startBundle
- finishBundle
- processElementForWindowObservingParDo
- processElementForParDo
There are some other places we also invoke user code:
- processElementForWindowObservingPairWithRestriction
- processElementForPairWithRestriction
- processElementForWindowObservingSplitRestriction
- processElementForSplitRestriction
- processElementForWindowObservingTruncateRestriction
- processElementForTruncateRestriction
- processElementForWindowObservingSizedElementAndRestriction
- tearDown
- invoke timer callback
| * TransformProcessingThreadTracker tracks the thread ids for the transforms that are being | ||
| * processed in the SDK harness. | ||
| */ | ||
| public class TransformProcessingThreadTracker { |
There was a problem hiding this comment.
Sorry that I'm not familiar with how logging service works, I'm wondering whether this will have multi-threading concurrency issue.
There was a problem hiding this comment.
I think there might be a very slight chance the processing thread moved onto another transform when the LogHandler haven't done transforming the log entries in previous one. But I think the it should be very rare(log transform should be almost instant) and I would argue that it's probably better to keep the logging just best effort instead of introducing locks to guarantee 100% metadata correctness?
There was a problem hiding this comment.
The argument for it's probably better to keep the logging just best effort is that it's ok to have step name mismatched with log message itself. Do you think it's acceptable when it happens?
There was a problem hiding this comment.
I'm not sure if it is ever gonna happen, we get to see if the integration test is flaky(in my 30+ IT test runs mismatch never happens). If the occurrence is less than 0.01% I don't think it'll have actual impact on usability. Current empty step in sdk logs have no values to users and can be considered almost 100% mismatch, so I think this PR should be at least an improvement to that.
There was a problem hiding this comment.
I agree that test signals can give us more confidence.
Current empty step in sdk logs have no values to users and can be considered almost 100% mismatch, so I think this PR should be at least an improvement to that.
I would say, We provide some information but they can be wrong is not better than We don't provide more information.
There was a problem hiding this comment.
I agree. I don't think the concurrency risk is ever gonna be an issue, we can tell from enabling the test and track the history(I also manually tested another 50 times and tests all passed with matching step). I believe we provide some information and they can be wrong in very rare cases would still be more valuable than don't provide the information and probably won't cause too much trouble for users, the element count in streaming pipeline falls into this best effort category as well.
Do you think the step info will be useful for the logs in SDF related callbacks? I haven't added for them since I'm not familiar how user would use logging in SDFs but sure we can add tracking for all of them if necessary. |
|
got it, I'll add it. |
|
Run Java PreCommit |
| public class TransformProcessingThreadTracker { | ||
| private static final TransformProcessingThreadTracker INSTANCE = | ||
| new TransformProcessingThreadTracker(); | ||
| private final ConcurrentHashMap<Long, String> threadIdToTransformIdMappings; |
There was a problem hiding this comment.
Another question is that will this map grow unlimitedly? I'm kind of concerning that it consumes too much memory with a long run instance(and the thread is not reused).
There was a problem hiding this comment.
hmm yeah I think you are right, it's potentially an issue and I've changed to use a LoadingCache with expiration.
.../java/harness/src/main/java/org/apache/beam/fn/harness/TransformProcessingThreadTracker.java
Outdated
Show resolved
Hide resolved
|
Run Java PreCommit |
|
Task :sdks:java:harness:checkstyleMain is failing. |
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:
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.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
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.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.