Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(868)

Issue 224610044: Minor bug fixes (inc. #169) and adding information to foreman. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 11 months ago by kiddi
Modified:
8 years, 10 months ago
Reviewers:
Joachim Metz
CC:
log2timeline-dev_googlegroups.com
Visibility:
Public.

Description

These files are missing unit tests: + tools/pshell.py

Patch Set 1 #

Patch Set 2 : Uploading changes made to code. #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -14 lines) Patch
M plaso/frontend/extraction_frontend.py View 1 chunk +5 lines, -0 lines 0 comments Download
M plaso/multi_processing/foreman.py View 2 chunks +21 lines, -10 lines 0 comments Download
M plaso/multi_processing/multi_process.py View 10 chunks +42 lines, -0 lines 7 comments Download
M tools/pinfo.py View 1 chunk +4 lines, -3 lines 2 comments Download
M tools/pshell.py View 1 2 chunks +1 line, -1 line 0 comments Download

Messages

Total messages: 4
kiddi
8 years, 11 months ago (2015-04-25 23:07:37 UTC) #1
kiddi
Code updated.
8 years, 11 months ago (2015-04-26 02:06:04 UTC) #2
kiddi
. https://codereview.appspot.com/224610044/diff/20001/tools/pinfo.py File tools/pinfo.py (right): https://codereview.appspot.com/224610044/diff/20001/tools/pinfo.py#newcode202 tools/pinfo.py:202: u'-d', u'--detail', dest=u'verbose', action=u'store_true', see that the pinfo ...
8 years, 11 months ago (2015-04-26 02:08:01 UTC) #3
Joachim Metz
8 years, 11 months ago (2015-04-26 07:48:19 UTC) #4
I don't agree with a lot of the changes. Also what are the reasons for some of
them?

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
File plaso/multi_processing/multi_process.py (right):

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:99: if not
self._IsWorkerMonitored(worker_process):
At this point this method is called the workers monitoring should not be
relevant and this not necessary.

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:128: continue
At this point this method is called the workers monitoring should not be
relevant and this not necessary.

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:148: if not
self._IsWorkerMonitored(worker_process):
At this point this method is called the workers monitoring should not be
relevant and this not necessary.

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:177: if not
self._IsWorkerMonitored(worker_process):
At this point this method is called the workers monitoring should not be
relevant and this not necessary.

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:189: def _IsWorkerMonitored(self,
worker_process):
Shouldn't this be in the foreman?

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:376: self._storage_writer_process.daemon
= True
The storage process is managed by main process why do you need to set daemon?

https://codereview.appspot.com/224610044/diff/20001/plaso/multi_processing/mu...
plaso/multi_processing/multi_process.py:418: # If the worker is not being
monitored there is no need to go through
NO, the main process needs to manage its child processes.

https://codereview.appspot.com/224610044/diff/20001/tools/pinfo.py
File tools/pinfo.py (right):

https://codereview.appspot.com/224610044/diff/20001/tools/pinfo.py#newcode202
tools/pinfo.py:202: u'-d', u'--detail', dest=u'verbose', action=u'store_true',
On 2015/04/26 02:08:01, kiddi wrote:
> see that the pinfo CL changes the default setting for version to -V instead of
> -v so this might not be a necessary change (but there might still be a
confusion
> between version and verbose, which is the same here as in detail vs. debug)

I opt to stick with verbose it is more widely used than detail.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b