You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "phet (via GitHub)" <gi...@apache.org> on 2023/12/21 01:08:43 UTC
[PR] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
phet opened a new pull request, #3850:
URL: https://github.com/apache/gobblin/pull/3850
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
Logging Task state collector failure at the granularity of every task is impractical, when tasks number in the 100k's. Moreover the stacktrace from retrieval failure provides no meaningful info--even more so when faced w/ thousands of repetitive logs like (pre-change):
```
2023/12/20 22:08:17.894 +0000 WARN [ParallelRunner] [Azkaban] Task failed: Deserialize state for task_name_of_job_task_goes_here_0_1703105111101_51.tst
java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
at java.util.ArrayList.rangeCheck(ArrayList.java:657)
at java.util.ArrayList.get(ArrayList.java:433)
at org.apache.gobblin.runtime.TaskStateCollectorService$2.call(TaskStateCollectorService.java:216)
at org.apache.gobblin.runtime.TaskStateCollectorService$2.call(TaskStateCollectorService.java:213)
at org.apache.gobblin.util.executors.MDCPropagatingCallable.call(MDCPropagatingCallable.java:42)
at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:111)
at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:58)
at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:75)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
```
Therefore only log sparsely (but present a running total) and omit the stack trace
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
existing unit tests
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-1979] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3850:
URL: https://github.com/apache/gobblin/pull/3850#discussion_r1435129018
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/TaskStateCollectorService.java:
##########
@@ -252,22 +255,32 @@ public boolean apply(String input) {
}
final Queue<TaskState> taskStateQueue = Queues.newConcurrentLinkedQueue();
+ AtomicLong numStateStoreMissing = new AtomicLong(0L);
+ GrowthMilestoneTracker growthTracker = new GrowthMilestoneTracker();
try (ParallelRunner stateSerDeRunner = new ParallelRunner(numDeserializerThreads, null)) {
for (final String taskStateName : taskStateNames) {
log.debug("Found output task state file " + taskStateName);
// Deserialize the TaskState and delete the file
stateSerDeRunner.submitCallable(new Callable<Void>() {
@Override
public Void call() throws Exception {
- TaskState taskState = taskStateStore.getAll(taskStateTableName, taskStateName).get(0);
- taskStateQueue.add(taskState);
+ List<TaskState> matchingTaskStates = taskStateStore.getAll(taskStateTableName, taskStateName);
+ if (matchingTaskStates.isEmpty()) {
Review Comment:
correct: this solely addresses cases where the state store does not retrieve the task state, but otherwise exits normally. perhaps in another sort of failure, a state store impl might throw. this consolidation still permits such failure to pass through uninterrupted.
since the state store already gave us the list of task state names on line 244, I'd expect any other such failure to be ephemeral (else an abject logical bug in the state store). either way, I've avoided over-engineering the solution, precisely, as you point out, because we'd lose valuable debugging info by conflating dissimilar errors.
if the a future failure scenario should arise from which we gain a concrete grasp on what kind of errors these might be, I'd suggest at that time to extend this solution.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-1979] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3850:
URL: https://github.com/apache/gobblin/pull/3850#discussion_r1435129018
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/TaskStateCollectorService.java:
##########
@@ -252,22 +255,32 @@ public boolean apply(String input) {
}
final Queue<TaskState> taskStateQueue = Queues.newConcurrentLinkedQueue();
+ AtomicLong numStateStoreMissing = new AtomicLong(0L);
+ GrowthMilestoneTracker growthTracker = new GrowthMilestoneTracker();
try (ParallelRunner stateSerDeRunner = new ParallelRunner(numDeserializerThreads, null)) {
for (final String taskStateName : taskStateNames) {
log.debug("Found output task state file " + taskStateName);
// Deserialize the TaskState and delete the file
stateSerDeRunner.submitCallable(new Callable<Void>() {
@Override
public Void call() throws Exception {
- TaskState taskState = taskStateStore.getAll(taskStateTableName, taskStateName).get(0);
- taskStateQueue.add(taskState);
+ List<TaskState> matchingTaskStates = taskStateStore.getAll(taskStateTableName, taskStateName);
+ if (matchingTaskStates.isEmpty()) {
Review Comment:
correct: this solely addresses cases where the state store does not retrieve the task state, but otherwise exits normally. perhaps in another sort of failure, a state store impl might throw. this is NOT subject to consolidation.
since the state store already gave us the list of task state names on line 244, I'd expect any other such failure to be ephemeral (else an abject logical bug in the state store). either way, I've avoided over-engineering the solution, precisely, as you point out, because we'd lose valuable debugging info by conflating dissimilar errors.
if the a future failure scenario should arise from which we gain a concrete grasp on what kind of errors these might be, I'd suggest at that time to extend this solution.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-1979] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo merged PR #3850:
URL: https://github.com/apache/gobblin/pull/3850
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3850:
URL: https://github.com/apache/gobblin/pull/3850#issuecomment-1867049926
## [Codecov](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `5 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`18fba9e`)](https://app.codecov.io/gh/apache/gobblin/commit/18fba9e5c3d70f21a7b1dbe2b65ee83c23bfa4e6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.60% compared to head [(`ea0cd42`)](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.09%.
| [Files](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...n/exception/RuntimeExceptionWithoutStackTrace.java](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2V4Y2VwdGlvbi9SdW50aW1lRXhjZXB0aW9uV2l0aG91dFN0YWNrVHJhY2UuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3850 +/- ##
============================================
- Coverage 47.60% 46.09% -1.52%
+ Complexity 11065 2182 -8883
============================================
Files 2159 417 -1742
Lines 85464 17997 -67467
Branches 9497 2194 -7303
============================================
- Hits 40682 8295 -32387
+ Misses 41080 8819 -32261
+ Partials 3702 883 -2819
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/gobblin/pull/3850?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3850:
URL: https://github.com/apache/gobblin/pull/3850#discussion_r1434588243
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/TaskStateCollectorService.java:
##########
@@ -252,22 +255,32 @@ public boolean apply(String input) {
}
final Queue<TaskState> taskStateQueue = Queues.newConcurrentLinkedQueue();
+ AtomicLong numStateStoreMissing = new AtomicLong(0L);
+ GrowthMilestoneTracker growthTracker = new GrowthMilestoneTracker();
try (ParallelRunner stateSerDeRunner = new ParallelRunner(numDeserializerThreads, null)) {
for (final String taskStateName : taskStateNames) {
log.debug("Found output task state file " + taskStateName);
// Deserialize the TaskState and delete the file
stateSerDeRunner.submitCallable(new Callable<Void>() {
@Override
public Void call() throws Exception {
- TaskState taskState = taskStateStore.getAll(taskStateTableName, taskStateName).get(0);
- taskStateQueue.add(taskState);
+ List<TaskState> matchingTaskStates = taskStateStore.getAll(taskStateTableName, taskStateName);
+ if (matchingTaskStates.isEmpty()) {
Review Comment:
This PR only reduces logs in the case where we are missing task states, but if multiple tasks have the same reason of failure they are still not pared down?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-1979] Pare down `TaskStateCollectorService` failure logging, to avoid flooding logs during widespread failure, e.g. O(1k)+ [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3850:
URL: https://github.com/apache/gobblin/pull/3850#discussion_r1435129018
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/TaskStateCollectorService.java:
##########
@@ -252,22 +255,32 @@ public boolean apply(String input) {
}
final Queue<TaskState> taskStateQueue = Queues.newConcurrentLinkedQueue();
+ AtomicLong numStateStoreMissing = new AtomicLong(0L);
+ GrowthMilestoneTracker growthTracker = new GrowthMilestoneTracker();
try (ParallelRunner stateSerDeRunner = new ParallelRunner(numDeserializerThreads, null)) {
for (final String taskStateName : taskStateNames) {
log.debug("Found output task state file " + taskStateName);
// Deserialize the TaskState and delete the file
stateSerDeRunner.submitCallable(new Callable<Void>() {
@Override
public Void call() throws Exception {
- TaskState taskState = taskStateStore.getAll(taskStateTableName, taskStateName).get(0);
- taskStateQueue.add(taskState);
+ List<TaskState> matchingTaskStates = taskStateStore.getAll(taskStateTableName, taskStateName);
+ if (matchingTaskStates.isEmpty()) {
Review Comment:
correct: this solely addresses cases where the state store does not retrieve the task state, but otherwise exits normally. perhaps in another sort of failure, a state store impl might throw. this consolidation still permits such failure to pass through uninterrupted.
since the state store already gave us the list of task state names on line 244, I'd expect any other such failure to be ephemeral (else an abject logical bug in the state store). either way, I've avoided over-engineering the solution, precisely, as you point out, because we'd lose valuable debugging info by conflating dissimilar errors.
if a future failure scenario should arise from which we gain a concrete grasp on what kind of errors these might be, I'd suggest at that time to extend this solution.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org