[plaso] Added processed directory to speed up listdir

[plaso] Added processed directory to speed up listdir

[onager, 2018-04-01 19:58:18]

On 2018/04/01 17:32:39, Joachim Metz wrote:

I don't think "to merge" is the name to go with here. Having "merge" and "to
merge" directory names is very confusing. From what I can tell, the idea of this
change was to reduce the amount of time spent iterating over filenames of
completed tasks, for situations where there are large number of files waiting to
merged. What sort of performance increase do you get? My profiling suggested
most of the time spent in the "pending merge" check were in string format()
calls, not listdir() calls, though clearly, removing the number of files to
iterate over will help. 

Memoizing task paths might be a less complex way of improving performance,
compared to adding a new directory and task state. We've had a lot of issues
nailing down different task transitions, so I'm not too excited about changing
them again.

[onager, 2018-04-01 19:58:26]

https://codereview.appspot.com/338670043/diff/1/plaso/multi_processing/task_m...
File plaso/multi_processing/task_manager.py (right):

https://codereview.appspot.com/338670043/diff/1/plaso/multi_processing/task_m...
plaso/multi_processing/task_manager.py:276: """Retrieves the task when it is
mergeable.
You seem to be adding a new state that a task can be in. Please update the task
docstring to reflect this, and define what this state means.

For consistency, please add "ByIdentifier" to the method name.

I'm also confused about the "when" in the docstring here - should it be "if"?

Shouldn't this also raise a KeyError if the task identifier isn't found, like
the other methods in this class?

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:968: """Prepares a task storage for pending merge.
This prepares a task storage for preparing to merge? This terminology is
confusing, perhaps differentiate between "completed" tasks where the worker is
done, and "pending merge" tasks, where the main process is waiting to process
them.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1406: """Retrieves the task identifiers which are
ready to be merged.
-the

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1451: Moves the task storage file from to merge
directory to the merge directory.
See note about terminology, this is a very confusing sentence to read.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1458: if the temporary path for the task storage does
not exist.
or if the storage file can't be renamed.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1478: def PrepareToMergeTaskStorage(self, task):
Having "PrepareMerge" and "PrepareToMerge" methods which do different things is
very confusing, please reconsider the naming here.

It's easy to tell which one of these needs to be called first.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1489: if the temporary path for the task storage does
not exist.
+or if the storage file can't be renamed.

[Joachim Metz, 2018-04-02 05:01:08]

https://codereview.appspot.com/338670043/diff/1/plaso/multi_processing/task_m...
File plaso/multi_processing/task_manager.py (right):

https://codereview.appspot.com/338670043/diff/1/plaso/multi_processing/task_m...
plaso/multi_processing/task_manager.py:276: """Retrieves the task when it is
mergeable.
Done.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:968: """Prepares a task storage for pending merge.
Changed

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1406: """Retrieves the task identifiers which are
ready to be merged.
On 2018/04/01 19:58:26, onager wrote:
> -the

Done.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1451: Moves the task storage file from to merge
directory to the merge directory.
On 2018/04/01 19:58:26, onager wrote:
> See note about terminology, this is a very confusing sentence to read.

Ack, reworded.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1458: if the temporary path for the task storage does
not exist.
On 2018/04/01 19:58:26, onager wrote:
> or if the storage file can't be renamed.

Done.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1478: def PrepareToMergeTaskStorage(self, task):
On 2018/04/01 19:58:26, onager wrote:
> Having "PrepareMerge" and "PrepareToMerge" methods which do different things
is
> very confusing, please reconsider the naming here.
> 
> It's easy to tell which one of these needs to be called first.

Done.

https://codereview.appspot.com/338670043/diff/1/plaso/storage/interface.py#ne...
plaso/storage/interface.py:1489: if the temporary path for the task storage does
not exist.
On 2018/04/01 19:58:26, onager wrote:
> +or if the storage file can't be renamed.

Done.

[Joachim Metz, 2018-04-02 05:13:40]

Code updated.

[onager, 2018-04-07 13:15:54]

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
File plaso/multi_processing/task_manager.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:107: * processed: a worker has completed
processing the task.
+ but it is not yet ready to be merged into the session storage.

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:108: * pending_merge: the task is process
and awaiting to be merged with the
"the task has been processed and is ready to be merged with the session storage"

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:276: def
GetProcessedTasksByIdentifier(self, task_identifier):
task, not tasks

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:277: """Retrieves a task when it
completed processing.
"Retrieves a task that has been processed".

But this method doesn't seem to do that, though. I don't see any code in this
method that checks if the task has been processed. Can you please clarify what
this method does?

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:283: Task: a task that is pending merge.
-is pending merge +has been processed

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:953: """Finalized a proccessed a task storage.
Finalizes

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1381: """Retrieves task identifiers which are
processed.
"Identifers for tasks which have been processed"

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1422: """Finalized a proccessed a task storage.
Finalizes a processed task storage

[onager, 2018-04-07 14:34:06]

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
File plaso/multi_processing/task_engine.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_engine.py:208: 'Unable to retrieve task: {0:s} to
prepare for merge.'.format(
"to prepare it to be merged"

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
File plaso/multi_processing/task_manager.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:99: following states:
This is now no longer true, as a task can now be both processed and abandoned. 

GetProcessedTaskByIdentifier doesn't update the state of the task it returns, so
we could still have issues/race conditions around retrying abandoned tasks,
updating processing status from workers etc.

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1397: path.replace('.plaso', '')
This is going to cause issues if there's ever a non-plaso file in the processed
task directory. Is that what we want? It seems quite fragile.

[Joachim Metz, 2018-04-07 16:49:28]

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
File plaso/multi_processing/task_engine.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_engine.py:208: 'Unable to retrieve task: {0:s} to
prepare for merge.'.format(
On 2018/04/07 14:34:06, onager wrote:
> "to prepare it to be merged"

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
File plaso/multi_processing/task_manager.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:99: following states:
when a task is processed and abandoned, then it was prematurely abandoned and
now is considered processed. I also don't see an explanation here for a retry
state?

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:107: * processed: a worker has completed
processing the task.
On 2018/04/07 13:15:53, onager wrote:
> + but it is not yet ready to be merged into the session storage.

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:108: * pending_merge: the task is process
and awaiting to be merged with the
On 2018/04/07 13:15:54, onager wrote:
> "the task has been processed and is ready to be merged with the session
storage"

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:276: def
GetProcessedTasksByIdentifier(self, task_identifier):
On 2018/04/07 13:15:53, onager wrote:
> task, not tasks

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:277: """Retrieves a task when it
completed processing.
I cannot follow your reasoning. What checks do you expect in this method given
the name of the method and other Get methods in this file?

https://codereview.appspot.com/338670043/diff/20001/plaso/multi_processing/ta...
plaso/multi_processing/task_manager.py:283: Task: a task that is pending merge.
On 2018/04/07 13:15:53, onager wrote:
> -is pending merge +has been processed

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:953: """Finalized a proccessed a task storage.
On 2018/04/07 13:15:54, onager wrote:
> Finalizes

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1381: """Retrieves task identifiers which are
processed.
On 2018/04/07 13:15:54, onager wrote:
> "Identifers for tasks which have been processed"

Done.

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1397: path.replace('.plaso', '')
this was introduced in
https://github.com/log2timeline/plaso/commit/a42174f22213f535800644a707c8aab9...
let me know what your considerations were. I'll keep it as and create and issue
to clean this up.

https://codereview.appspot.com/338670043/diff/20001/plaso/storage/interface.p...
plaso/storage/interface.py:1422: """Finalized a proccessed a task storage.
On 2018/04/07 13:15:54, onager wrote:
> Finalizes a processed task storage

Done.

[Joachim Metz, 2018-04-07 16:55:40]

Code updated.

[Joachim Metz, 2018-04-07 17:34:47]

Code updated.

[Joachim Metz, 2018-04-24 04:23:58]

Code updated.

[Joachim Metz, 2018-04-24 04:30:24]

Code updated.

[onager, 2018-05-01 08:52:51]

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/fake/write...
File plaso/storage/fake/writer.py (right):

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/fake/write...
plaso/storage/fake/writer.py:367: """Finalized a proccessed a task storage.
"Finalizes" and "processed"

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface....
plaso/storage/interface.py:964: """Finalizes a proccessed a task storage.
-one a

Also rename to "FinalizeTaskStorage", since by the definitions in the task
manager, a task isn't "processed" until after it's been finalized (moved to
processed directory).

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface....
plaso/storage/interface.py:1444: """Finalized a proccessed a task storage.
"finalizes" and "processed".

[Joachim Metz, 2018-05-01 10:21:39]

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/fake/write...
File plaso/storage/fake/writer.py (right):

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/fake/write...
plaso/storage/fake/writer.py:367: """Finalized a proccessed a task storage.
On 2018/05/01 08:52:51, onager wrote:
> "Finalizes" and "processed"

Done.

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface.py
File plaso/storage/interface.py (right):

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface....
plaso/storage/interface.py:964: """Finalizes a proccessed a task storage.
On 2018/05/01 08:52:51, onager wrote:
> -one a
> 
> Also rename to "FinalizeTaskStorage", since by the definitions in the task
> manager, a task isn't "processed" until after it's been finalized (moved to
> processed directory).

Done.

https://codereview.appspot.com/338670043/diff/100001/plaso/storage/interface....
plaso/storage/interface.py:1444: """Finalized a proccessed a task storage.
On 2018/05/01 08:52:51, onager wrote:
> "finalizes" and "processed".

Done.

[Joachim Metz, 2018-05-01 10:26:36]

Code updated.

[onager, 2018-05-01 15:09:53]

LGTM

[Joachim Metz, 2018-05-01 16:10:17]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: review.py close dbmerge2