Issue 345920043: [plaso] Changes to handle retry task (Closed)
|
Created:
6 years ago by Joachim Metz Modified:
5 years, 11 months ago Reviewers:
onager CC:
kiddi, log2timeline-dev_googlegroups.com, romaing Visibility:
Public. |
Description[plaso] Changes to handle retry task
Patch Set 1 : Small improvements #
Total comments: 9
Patch Set 2 : Changes after review and merge #
Total comments: 25
Patch Set 3 : Changes after review #Patch Set 4 : Changes after review #
Total comments: 4
Patch Set 5 : Changes after review #
Total comments: 4
Patch Set 6 : Changes after review #
Total comments: 1
Patch Set 7 : Additional changes to remove abandoned tasks that no longer need tracking #
Total comments: 2
MessagesTotal messages: 26
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
A couple of early comments, but I haven't finished reviewing all the changes yet. https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:32: task is a retry or None if not set. -if not set +is not a retry of another task. https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:35: retry_task_identifier (str): identifier of the retry task, when the task I'm not sure what this means, please rephrase. I recommend avoiding the phrase "retry task". I guess this an identifier of a task which is an attempt to retry this task? If so, how does that work with a retry count? If a task is retried more than once, what is this set to? https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:82: task (Task): a task to retry of the existing task. Please correct the docstring format, this looks like an argument docstring. "a task to retry of" doesn't make sense either. Perhaps just revert to the original string here. https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. Completed tasks aren't tracked by the manager, per the paragraph after this one.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:32: task is a retry or None if not set. On 2018/05/01 16:09:37, onager wrote: > -if not set +is not a retry of another task. `is not a retry of another task.` no this is only 1 specific case why original_task_identifier would be None. Another case is when Task is initialized, hence the rewording. https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:35: retry_task_identifier (str): identifier of the retry task, when the task `I'm not sure what this means, please rephrase. I recommend avoiding the phrase "retry task".` this adds to confusion. what do you mean with "retry" * at the highest level plaso retries processing the path spec * at the task level that a single retry is expressed in a task ??? https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:82: task (Task): a task to retry of the existing task. no because "attempt" does not make sense either in the context of the task manager. maybe: "the retry task is a new task that allows the foreman to retry processing the path spec of a previously abandoned task." https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/60001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. not sure what you imply with this comment seeing it is non-actionable, but I've extended the docstring assuming that is what you meant.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/60001/plaso/containers/tasks.py... plaso/containers/tasks.py:35: retry_task_identifier (str): identifier of the retry task, when the task On 2018/05/01 17:03:30, Joachim Metz wrote: > `I'm not sure what this means, please rephrase. I recommend avoiding the phrase > "retry task".` > > this adds to confusion. what do you mean with "retry" > > * at the highest level plaso retries processing the path spec > * at the task level that a single retry is expressed in a task > > ??? I don't understand your reply here. Perhaps what this docstring is meant to be is: "identifier of a task which is an attempt to retry this task. If the original task has been retried multiple times, this will be the identifier of the last task created. None if this task has not been retried." Is that correct? Either way, please rephrase this docstring. https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. Adding multiple retry attempts adds a lot of complexity, please consider removing this functionality at this time. https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:82: task (Task): a task to retry of the existing task. Please correct the docstring format. https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:82: task (Task): a task to retry of the existing task. "a task to retry of..." doesn't make sense, please rewrite. On your proposed wording, and regarding "attempt", perhaps the docstring should capture that a retry is a essentially a copy of the original task, but with a different identifier and status information? https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. The task manager will More actionable: remove this line, completed tasks are not tracked by the manager. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:145: self._number_of_retry_tasks = 0 Why is this variable here? It's incremented, but never read/checked. Please remove it, as it's not used in the class. If you want it for the tests, please either add a comment describing why its here, or make it a publicly accessible value. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:220: if (not task.retry_task_identifier and Per our discussion on the retry_task_identifier attribute in the Task definition, your view is that this being set doesn't indicate whether a task has been retried or not. Please either: 1) Change the docstring explaining the attribute so that its meaning is clear; or 2) Use another method to determine if the task is eligible for to be retried https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:355: del self._tasks_abandoned[abandoned_task_identifier] Why is this necessary, removing an abandoned task if a retry was successful? If an abandoned task is later completed, this is going to raise, so I think this going to cause a race condition. Also, what state is the task in after its been removed from _tasks_abandoned? Is it completed? If so, the paragraph about completed above needs to change. That said, I still don't think the task should be removed from abandoned tasks. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:371: if not task.retry_task_identifier] See comment above about using the attribute to determine if a task has been retried. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:501: raise KeyError('Unable to merge a task {0:s} with retry task.'.format( -Unable +Will not
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. can you be more specific what more complexity it adds? IMHO the only thing different is an int instead of a bool to track if a task is a retry. Adding the retry_task_identifier is needed to fix a state inconsistency issue. https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:82: task (Task): a task to retry of the existing task. "Please correct the docstring format." unclear what you mean with this, as requested before, please be verbose in your review comments and make sure comments are actionable. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. The task manager will let's keep it for now, and determine what the role of the task manager should be seeing the recent surfaced task state inconsistencies https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:145: self._number_of_retry_tasks = 0 ack https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:220: if (not task.retry_task_identifier and per other conversation, let's keep as-is proposed and define in a doc first how this state should look like and then how to change the code accordingly. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:355: del self._tasks_abandoned[abandoned_task_identifier] > I still don't think the task should be removed from abandoned tasks. not removing, will blow up memory usage with high number of tasks > Why is this necessary as explained in person; to prevent event sources being processed multiple times (by different tasks) seeing results are still merged with the session storage https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:371: if not task.retry_task_identifier] Let's keep as proposed, what the stated "retried" and "retry" mean. This is closest to the original approach. Also the creation of a "retry task" does not mean the action has "been retried", which is another potential state inconsistency. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:501: raise KeyError('Unable to merge a task {0:s} with retry task.'.format( On 2018/05/06 09:32:04, onager wrote: > -Unable +Will not Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. On 2018/05/07 04:52:52, Joachim Metz wrote: > can you be more specific what more complexity it adds? IMHO the only thing > different is an int instead of a bool to track if a task is a retry. Adding the > retry_task_identifier is needed to fix a state inconsistency issue. Some pieces of complexity: * Some component needs to know how many retries is "too many" or "not enough", and it's not clear where this value should be stored * There's no single, obvious way to check whether a task is a retry or not, per my other comments about retry_task_identifier * It's not clear which identifier of possibly multiple task retries is stored in retry_task_identifier https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. The task manager will On 2018/05/07 04:52:52, Joachim Metz wrote: > let's keep it for now, and determine what the role of the task manager should be > seeing the recent surfaced task state inconsistencies I don't agree, there's already a paragraph below describing what happens when a task is completely merged, there's no need to add this duplication. https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:355: del self._tasks_abandoned[abandoned_task_identifier] On 2018/05/07 04:52:52, Joachim Metz wrote: > > I still don't think the task should be removed from abandoned tasks. > > not removing, will blow up memory usage with high number of tasks A task isn't a particularly large object, how many abandoned tasks are you seeing? > > > Why is this necessary > > as explained in person; to prevent event sources being processed multiple times > (by different tasks) seeing results are still merged with the session storage Doesn't the extra check of CheckTaskToMerge() deal with that, rather than removing the task from _tasks_abandoned. From previous comment: what state is the task in after its been removed from _tasks_abandoned? Is it completed? If so, the paragraph about completed above needs to change. That said, I still don't think the task should be removed from abandoned tasks. From previous comment: If an abandoned task is later completed, this is going to raise, so I think this going to cause a race condition. https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.p... plaso/containers/tasks.py:85: task (Task): a task to retry of the existing task. The style guide: https://github.com/log2timeline/plaso/wiki/Style-guide has documentation has documentation on how to write a "Returns" docstring. This docstring was formatted correctly before this change, so you can reference that, per my previous comment if you like. The variable returned has no name. https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.p... plaso/containers/tasks.py:85: task (Task): a task to retry of the existing task. "a task to retry of..." doesn't make sense, please rewrite.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.p... plaso/containers/tasks.py:85: task (Task): a task to retry of the existing task. On 2018/05/07 19:59:51, onager wrote: > The style guide: https://github.com/log2timeline/plaso/wiki/Style-guide has > documentation has documentation on how to write a "Returns" docstring. This > docstring was formatted correctly before this change, so you can reference that, > per my previous comment if you like. The variable returned has no name. Acknowledged. https://codereview.appspot.com/345920043/diff/120001/plaso/containers/tasks.p... plaso/containers/tasks.py:85: task (Task): a task to retry of the existing task. ack, just FYI if you'd written: change "task (Task):" into "Task:" that would have been more clear and to the point
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
On 2018/05/08 04:30:29, Joachim Metz wrote: > Code updated. Just a quick note that I had a lot of other comments in #12, so I'll wait to review this until you've responded to those.
Sign in to reply to this message.
> Just a quick note that I had a lot of other comments in #12, so I'll wait to > review this until you've responded to those. As I've indicated before to you, rietveld UX does not help with commenting on previous patch sets. I've answered open comments that I could identify but please for the sake of clarity comment on the latest patch set.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. > Some component needs to know how many retries is "too many" or "not enough", and it's not clear where this value should be stored not sure I understand your point, the only thing different would be that there now is an explicit limit of retries, namely 2, instead of an implicit limit of 1 > There's no single, obvious way to check whether a task is a retry or not, per my other comments about retry_task_identifier I can re-add the bool if that solves this in your opinion > It's not clear which identifier of possibly multiple task retries is stored in retry_task_identifier ack, I can rename it to last_retry_task_identifier and last_original_task_identifier if that make is more clear for you https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:108: * completed: a task that has successfully been merged. The task manager will ack https://codereview.appspot.com/345920043/diff/80001/plaso/multi_processing/ta... plaso/multi_processing/task_manager.py:355: del self._tasks_abandoned[abandoned_task_identifier] > A task isn't a particularly large object, how many abandoned tasks are you seeing? many thousands observed behavior described here: https://github.com/log2timeline/plaso/issues/1832 > If an abandoned task is later completed, this is going to raise, so I think this going to cause a race condition. no, only the last retry task is allowed to complete, previously abandon tasks are ignored. One potential issue here is that if the last retry task completes before a previously abandoned task is considered processed. I've added this to the doc.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. On 2018/05/10 16:06:30, Joachim Metz wrote: > > Some component needs to know how many retries is "too many" or "not enough", > and it's not clear where this value should be stored > > not sure I understand your point, the only thing different would be that there > now is an explicit limit of retries, namely 2, instead of an implicit limit of 1 I disagree that's there's an "implicit limit". With a boolean, the task has either been retried, or not. Other components can know that property of the task and act accordingly. With a counter, other components need to look at the counter as well as some limit on how many retries is "too many". > > > There's no single, obvious way to check whether a task is a retry or not, per > my other comments about retry_task_identifier > > I can re-add the bool if that solves this in your opinion Yes, please re-add the bool and remove the counter. > > > It's not clear which identifier of possibly multiple task retries is stored in > retry_task_identifier > > ack, I can rename it to last_retry_task_identifier and > last_original_task_identifier if that make is more clear for you It would only be last_retry_task_identifier, right? original_task_identifier doesn't change, once set. As above though, I think keeping the existing boolean is a better option for now. https://codereview.appspot.com/345920043/diff/140001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/140001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:243: def _UpdateLastestProcessingTime(self, task): Latest https://codereview.appspot.com/345920043/diff/140001/tests/multi_processing/t... File tests/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/140001/tests/multi_processing/t... tests/multi_processing/task_manager.py:272: # Test with missing task. Please expand this comment to clarify what this is testing. What's a "missing" task? Is this a new state? Is this a supported/expected behavior? See comment on the task manager about the removal of tasks from _tasks_abandoned. Please also check for other instances of "missing task" in this file.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py File plaso/containers/tasks.py (right): https://codereview.appspot.com/345920043/diff/80001/plaso/containers/tasks.py... plaso/containers/tasks.py:34: retry_count (int): number of times the task has been retried. > I think keeping the existing boolean is a better option for now. so "retried" is not an accurate representation per definition in the document. I've remove original_task_identifier since how it is used introduces another state discrepancy. > I disagree that's there's an "implicit limit". There is a limit of 1 retry, which is suggested though not directly expressed, hence the term implicit. https://codereview.appspot.com/345920043/diff/140001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/140001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:243: def _UpdateLastestProcessingTime(self, task): On 2018/05/13 13:49:41, onager wrote: > Latest Done. https://codereview.appspot.com/345920043/diff/140001/tests/multi_processing/t... File tests/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/140001/tests/multi_processing/t... tests/multi_processing/task_manager.py:272: # Test with missing task. changed to "status of task is unknown" as terminology used in task manager.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/160001/plaso/multi_processing/t... File plaso/multi_processing/task_engine.py (right): https://codereview.appspot.com/345920043/diff/160001/plaso/multi_processing/t... plaso/multi_processing/task_engine.py:206: storage_writer.RemoveProcessedTaskStorage(task) TODO: remove abandoned task from task manager
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
One typo remains, please check for any other usage of "lastest" anywhere in this CL. LGTM https://codereview.appspot.com/345920043/diff/180001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/180001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:237: -1 white line rename method from "lastest" to "latest"
Sign in to reply to this message.
https://codereview.appspot.com/345920043/diff/180001/plaso/multi_processing/t... File plaso/multi_processing/task_manager.py (right): https://codereview.appspot.com/345920043/diff/180001/plaso/multi_processing/t... plaso/multi_processing/task_manager.py:237: On 2018/05/14 18:06:50, onager wrote: > -1 white line > > rename method from "lastest" to "latest" 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 dbmerge6
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
