Issue 342020043: [plaso] Changed when queued tasks are abandoned (Closed)
|
Created:
5 years, 12 months ago by Joachim Metz Modified:
5 years, 11 months ago CC:
kiddi, log2timeline-dev_googlegroups.com Visibility:
Public. |
Description[plaso] Changed when queued tasks are abandoned
Patch Set 1 : Renamed GetRetryTask to CreateRetryTask and clean up of task manager #
Total comments: 2
Patch Set 2 : Changes after merge #Patch Set 3 : Changes after review #
Total comments: 18
Patch Set 4 : Changes after review #
Total comments: 15
Patch Set 5 : Changes after review #
Total comments: 8
Patch Set 6 : Changes after review #
Total comments: 6
MessagesTotal messages: 20
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/20001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/20001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:248: def _UpdateProcessingTimeOfTask(self, task): If I understand this correctly, it looks like this updated the managers "processing time" based on the task "processing time". If that is the case, I would call this "_UpdateProcessingTimeFromTask".
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/20001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/20001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:248: def _UpdateProcessingTimeOfTask(self, task): done
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:104: * failed: a task that was abandoned and has no retry task identifier. Please rephrase this to explain what "no retry task identifier" means. Does this mean the task was a retry, or was not a retry? Please also update the definition of "abandoned" to indicate how an abandoned task is different to a failed task, as the definitions seem to overlap, and the sentence before indicates that all tasks are in exactly one of these states. Perhaps you want to add a new paragraph defining this new "failed" state, as for "pending" below? https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:138: # The most recent last processing time of a task. Change to "the latest processing time observed in a task". Additionally, the definition here also seems odd, because this is initialized to a time that doesn't relate to a task. Shouldn't this be None? https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:179: """Marks queued tasks abandoned if all tasks exceed the inactive time. All what tasks? All queued tasks? Please clarify in the docstring what this method does. This seems to abandon queued tasks if the manager hasn't observed a task in the processing state recently. Please move the "inactivity check" out of this method, to separate the concerns of abandoning tasks from deciding if the tasks should be abandoned. It would also be useful to have a logging message about that "inactive state" being triggered, so please add that to the method. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:246: def _UpdateProcessingTimeFromTask(self, task): Please revert this - this method does two different things that are only slightly related, and the update of the task is not clear from the method name. Please add a separate _UpdateLastProcessingTime(task) method and call that instead, so that it's clear what's being updated when. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:333: Failed tasks are tasks that were abandoned and have no retry task -"no retry task identifier" +"and have not been retried" This definition is also counter intuitive to me, I would have assumed that a failed task was a task to process a pathspec that failed even though it was retried. What are you trying to capture here? This seems to be an inversion of the existing behavior. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:482: self._UpdateProcessingTimeFromTask(task) As above, please revert this and add a separate call to update the last task processing time. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:524: self._UpdateProcessingTimeFromTask(task_processing) As above, please revert this and add a separate call to update the last task processing time. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:533: self._UpdateProcessingTimeFromTask(task_queued) As above, please revert this and add a separate call to update the last task processing time. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:542: self._UpdateProcessingTimeFromTask(task_abandoned) As above, please revert this and add a separate call to update the last task processing time.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:104: * failed: a task that was abandoned and has no retry task identifier. "no retry task identifier", this leaked in from a future change. Done https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:138: # The most recent last processing time of a task. It could be None then we have to special case handle this. If you find this that confusing I'll update the comment. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:179: """Marks queued tasks abandoned if all tasks exceed the inactive time. what do you mean with the second part? remove line 188: if self._task_last_processing_time < inactive_time: ? if so why? otherwise unclear to me what you mean per chat: let's keep the method as is for now and revisit when the tests are running again and we have better data on the when queued tasks should be timed out https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:246: def _UpdateProcessingTimeFromTask(self, task): On 2018/05/01 08:31:43, onager wrote: > Please revert this - this method does two different things that are only > slightly related, and the update of the task is not clear from the method name. > > Please add a separate _UpdateLastProcessingTime(task) method and call that > instead, so that it's clear what's being updated when. Done. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:333: Failed tasks are tasks that were abandoned and have no retry task On 2018/05/01 08:31:43, onager wrote: > -"no retry task identifier" +"and have not been retried" > > This definition is also counter intuitive to me, I would have assumed that a > failed task was a task to process a pathspec that failed even though it was > retried. What are you trying to capture here? This seems to be an inversion of > the existing behavior. Done. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:482: self._UpdateProcessingTimeFromTask(task) On 2018/05/01 08:31:43, onager wrote: > As above, please revert this and add a separate call to update the last task > processing time. Done. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:524: self._UpdateProcessingTimeFromTask(task_processing) On 2018/05/01 08:31:43, onager wrote: > As above, please revert this and add a separate call to update the last task > processing time. Done. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:533: self._UpdateProcessingTimeFromTask(task_queued) On 2018/05/01 08:31:43, onager wrote: > As above, please revert this and add a separate call to update the last task > processing time. Done. https://codereview.appspot.com/342020043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:542: self._UpdateProcessingTimeFromTask(task_abandoned) On 2018/05/01 08:31:43, onager wrote: > As above, please revert this and add a separate call to update the last task > processing time. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_engine.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_engine.py:432: message='Worker failed to process path specification', It looks like you want to invert the logic here, to indicate that a worker failed to process a pathspec, even though the pathspec might have been successfully processed later. What's the rationale here? It think is likely to be confusing for a user to see an error saying a pathspec wasn't processed, as well as (possibly) events from that path. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:120: Tasks are considered "failed" when they were abandoned and do not have -"and do not..." +"have not been retried", to match the definition in GetFailedTasks. This means that a task can become not-failed, right? If an abandoned task is later retried? I'm not convinced that "failed" is the correct word to use for this state, failed sounds very final to me. It's also strange to have tasks that aren't failed that haven't succeeded. Are you sure you don't want to call this "nonretriable" or similar, and change the logic? https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:140: # The last processing time observed in a task. This value is set to -last +latest, because an older processing time observed more recently is not set here. For example, if _UpdateLastProcessingTime is called once with a task with processing time 4, then subsequently with a task with processing time 2, this value should be 4, not 2. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:182: """Marks queued tasks abandoned if all queued tasks exceed inactive time. This less clear than before, this method doesn't check the inactivity time of any of the queued tasks. I don't see why we'd want to keep this method, vs: 1) moving lines 188-191 in the the new code to a new method _CheckAbandonQueuedTasks 2) renaming this method to _AbandonedQueuedTasks 3) updating HasPendingTasks accordingly https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:448: # are idle and the task has been queued for longer than the timeout. Please update this comment, as the code now no longer cares how long an individual task has been queued for.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_engine.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_engine.py:432: message='Worker failed to process path specification', > It looks like you want to invert the logic here ? not sure where do you base that on? I'm moving the logic into the task manager. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:120: Tasks are considered "failed" when they were abandoned and do not have > "have not been retried" no changing this to the proposed wording would result in more ambiguous wording, since it can be that the path spec has been retried (previous tasks have been retried). > This means that a task can become not-failed, right? If an abandoned task is later retried? correct see follow up changes in https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... line 346 > I'm not convinced that "failed" is the correct word to use for this state, failed sounds very final to me. it is final > Are you sure you don't want to call this "nonretriable" or similar, and change the logic? no that resembles retry to closely > It's also strange to have tasks that aren't failed that haven't succeeded. unclear what you mean with this comment, I'm considering it non-actionable https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:140: # The last processing time observed in a task. This value is set to On 2018/05/01 15:37:54, onager wrote: > -last +latest, because an older processing time observed more recently is not > set here. > > For example, if _UpdateLastProcessingTime is called once with a task with > processing time 4, then subsequently with a task with processing time 2, this > value should be 4, not 2. Done. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:182: """Marks queued tasks abandoned if all queued tasks exceed inactive time. Let's do this at a later stage. 1. get the test working again 2. measure task state duration 3. clean up the task manager further (seeing there are a couple of 1 line methods only used in HasPendingTasks that can be consolidated as well) https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:448: # are idle and the task has been queued for longer than the timeout. On 2018/05/01 15:37:54, onager wrote: > Please update this comment, as the code now no longer cares how long an > individual task has been queued for. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:182: """Marks queued tasks abandoned if all queued tasks exceed inactive time. I've created https://github.com/log2timeline/plaso/issues/1867. Seeing that this introduces a state discrepancy between the foreman (queue) and the task manager, I'm not convinced queued task abandonment is the right approach.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:120: Tasks are considered "failed" when they were abandoned and do not have On 2018/05/01 16:33:04, Joachim Metz wrote: > > "have not been retried" > > no changing this to the proposed wording would result in more ambiguous wording, > since it can be that the path spec has been retried (previous tasks have been > retried). > > > This means that a task can become not-failed, right? If an abandoned task is > later retried? > > correct see follow up changes in > https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... > line 346 > > > I'm not convinced that "failed" is the correct word to use for this state, > failed sounds very final to me. > > it is final It's not final, you just replied above that a task can become not-failed. > > > Are you sure you don't want to call this "nonretriable" or similar, and change > the logic? > > no that resembles retry to closely > > > It's also strange to have tasks that aren't failed that haven't succeeded. > > unclear what you mean with this comment, I'm considering it non-actionable More actionable: This definition for failed is too confusing, please either rename it or remove this state from the task manager. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:182: """Marks queued tasks abandoned if all queued tasks exceed inactive time. On 2018/05/02 04:22:20, Joachim Metz wrote: > I've created https://github.com/log2timeline/plaso/issues/1867. Seeing that this > introduces a state discrepancy between the foreman (queue) and the task manager, > I'm not convinced queued task abandonment is the right approach. Per comment on that issue, I don't believe there's a sufficiently reliable way to do this when factoring in worker crashes, networking failures etc. Please: 1) move lines 188-191 in the the new code to a new method _CheckAbandonQueuedTasks 2) rename this method to _AbandonedQueuedTasks 3) update HasPendingTasks accordingly https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... File plaso/multi_processing/task_engine.py (right): https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_engine.py:439: for task in self._task_manager.GetFailedTasks(): OK, I see what you're doing here now, but GetFailedTasks() only produces that same result here once the task manager is done with all pending tasks. https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:144: self._task_last_processing_time = int( -last +latest, and for all other usages of this https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:251: def _UpdateLastProcessingTime(self, task): Rename from Last->Latest https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:449: # when it pops a task from the queue. Hence we check for tasks that we Please update this comment, as the code no longer checks any quality of the queued tasks. Also, this comment can be dramatically shortened if there's an obvious checking of a conditional here, as I've requested.
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:120: Tasks are considered "failed" when they were abandoned and do not have per conversation, keep it failed for now and revise after we sort out the names and meaning of the states. https://codereview.appspot.com/342020043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:182: """Marks queued tasks abandoned if all queued tasks exceed inactive time. On 2018/05/04 13:33:58, onager wrote: > On 2018/05/02 04:22:20, Joachim Metz wrote: > > I've created https://github.com/log2timeline/plaso/issues/1867. Seeing that > this > > introduces a state discrepancy between the foreman (queue) and the task > manager, > > I'm not convinced queued task abandonment is the right approach. > > Per comment on that issue, I don't believe there's a sufficiently reliable way > to do this when factoring in worker crashes, networking failures etc. Please: > 1) move lines 188-191 in the the new code to a new method > _CheckAbandonQueuedTasks > 2) rename this method to _AbandonedQueuedTasks > 3) update HasPendingTasks accordingly Acknowledged. https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... File plaso/multi_processing/task_engine.py (right): https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_engine.py:439: for task in self._task_manager.GetFailedTasks(): (I assume this is an informational comment?) https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:144: self._task_last_processing_time = int( On 2018/05/04 13:33:59, onager wrote: > -last +latest, and for all other usages of this Done. https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:251: def _UpdateLastProcessingTime(self, task): On 2018/05/04 13:33:59, onager wrote: > Rename from Last->Latest Done. https://codereview.appspot.com/342020043/diff/100001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:449: # when it pops a task from the queue. Hence we check for tasks that we On 2018/05/04 13:33:59, onager wrote: > Please update this comment, as the code no longer checks any quality of the > queued tasks. > > Also, this comment can be dramatically shortened if there's an obvious checking > of a conditional here, as I've requested. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Some small docstring nits+suggestions, LGTM https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:123: when the foreman is done processing. This is ok, but rather than reference the foreman and processing, "failed" tasks are valid once there are no pending tasks, which constrains the definition to things are a defined in the task manager. https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:329: Failed tasks are tasks that were abandoned and have not been retried. +once the foreman has finished processing all pending tasks. https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:451: # update to from a worker. -to
Sign in to reply to this message.
https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:123: when the foreman is done processing. ack https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:329: Failed tasks are tasks that were abandoned and have not been retried. On 2018/05/06 09:20:36, onager wrote: > +once the foreman has finished processing all pending tasks. Ack https://codereview.appspot.com/342020043/diff/120001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:451: # update to from a worker. On 2018/05/06 09:20:36, onager wrote: > -to Done.
Sign in to reply to this message.
Changes have been merged with master branch. To close the review and clean up the feature branch you can run: review.py close dbmerge5
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
