Issue 340610043: [plaso] SQLite merge reader added zlib support and clean up (Closed)
|
Created:
6 years ago by Joachim Metz Modified:
5 years, 11 months ago Reviewers:
onager CC:
kiddi, log2timeline-dev_googlegroups.com, jberggren, romaing Visibility:
Public. |
Description[plaso] SQLite merge reader added zlib support and clean up
Patch Set 1 : Small textual changes #
Total comments: 24
Patch Set 2 : Changes after review #
Total comments: 4
Patch Set 3 : Changes after merge #Patch Set 4 : Changes after review #
Total comments: 6
Patch Set 5 : Changes after review #
MessagesTotal messages: 18
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.py File plaso/storage/interface.py (right): https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1081: def _GetMergeTaskStorageFilePath(self, task): Rather than doing this, how about moving these variables to the task itself? Some preliminary profiling shows a lot of time spent in format() calls, hence https://github.com/log2timeline/dfvfs/issues/288 https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1082: """Retrieves the path of a task storage file ready to be merged. Do you mean "a task that is a ready to be merged"? Please amend the docstring. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1085: task (Task): task. Is this a file that the caller has already determined to be ready to merged? If so, please add this to the docstring. What happens if the task isn't ready to be merged? https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1094: """Retrieves the path of a task storage file. Based on the method above, this is task that isn't ready to be merged, correct? If so, please amend the docstring. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:75: container_type: getattr(self, method_name, None) Why use getattr here? A missing method here seems to suggest a coding error. It looks a missing method here will result in a runtime error, but this doesn't seem to be a runtime issue, unless there's some other dynamic method generation I'm missing? https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:154: list[str]: names of the container types to merge. This the names of the container types in the task storage being merged, right? You can't have container types in the store that aren't being merged? If so, please change the docstring to make that clear. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:160: # the container types list in order. Why is the order of the container types important? Please add the reason to the docstring. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:161: container_types = list(self._CONTAINER_TYPES) List comprehension looks better here: container_types = [table_name for table_name in table_names if table_name in self._CONTAINER_TYPE] https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:168: """Determines the next active container type.""" This seems to do more than just determine the type, it configures the class to merge that container type, as far as I can see. Please rename the method and adjust the docstring. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:195: bool: True if the storage metadata was read. This method doesn't return True under any conditions. Please fix. https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... File tests/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... tests/storage/sqlite/merge_reader.py:62: test_reader._ReadStorageMetadata() This looks to be missing an "assertTrue", please fix. Or maybe this method isn't supposed to return? See separate comment about the docstring not matching the behavior on that method. https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... tests/storage/sqlite/merge_reader.py:82: test_reader.MergeAttributeContainers() This looks to be missing an assertTrue, please fix.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.py File plaso/storage/interface.py (right): https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1081: def _GetMergeTaskStorageFilePath(self, task): Let's do one (optimization) step at a time. Also the task does not know anything about where it is stored. Your proposal sounds like a possible convolution of concerns. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1082: """Retrieves the path of a task storage file ready to be merged. No I don't mean this. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1085: task (Task): task. at this layer this concern is unknown it is a task storage file in the merge directory. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/interface.p... plaso/storage/interface.py:1094: """Retrieves the path of a task storage file. same point as before, you're proposing to mix high and low level concerns. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:75: container_type: getattr(self, method_name, None) The only way to do this with the method it self is after the method is decelerated. I can generate RuntimeError instead. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:154: list[str]: names of the container types to merge. On 2018/04/01 19:18:05, onager wrote: > This the names of the container types in the task storage being merged, right? > You can't have container types in the store that aren't being merged? If so, > please change the docstring to make that clear. Done. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:160: # the container types list in order. On 2018/04/01 19:18:05, onager wrote: > Why is the order of the container types important? Please add the reason to the > docstring. Done. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:161: container_types = list(self._CONTAINER_TYPES) On 2018/04/01 19:18:05, onager wrote: > List comprehension looks better here: > container_types = [table_name for table_name in table_names if table_name in > self._CONTAINER_TYPE] Done. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:168: """Determines the next active container type.""" On 2018/04/01 19:18:05, onager wrote: > This seems to do more than just determine the type, it configures the class to > merge that container type, as far as I can see. Please rename the method and > adjust the docstring. Done. https://codereview.appspot.com/340610043/diff/20001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:195: bool: True if the storage metadata was read. On 2018/04/01 19:18:05, onager wrote: > This method doesn't return True under any conditions. Please fix. Done. https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... File tests/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... tests/storage/sqlite/merge_reader.py:62: test_reader._ReadStorageMetadata() On 2018/04/01 19:18:05, onager wrote: > This looks to be missing an "assertTrue", please fix. Or maybe this method isn't > supposed to return? See separate comment about the docstring not matching the > behavior on that method. Done. https://codereview.appspot.com/340610043/diff/20001/tests/storage/sqlite/merg... tests/storage/sqlite/merge_reader.py:82: test_reader.MergeAttributeContainers() On 2018/04/01 19:18:05, onager wrote: > This looks to be missing an assertTrue, please fix. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:165: as event referencing event data. To resulting names are ordered to ensure -To resulting It sounds like the ordering only solves this problem because container types that are referenced by other containers happen to be lower in alphabetical order, is that correct? https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:194: """Prepars for the next container type. Prepars +Prepares
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:165: as event referencing event data. To resulting names are ordered to ensure no, event source comes before event data https://codereview.appspot.com/340610043/diff/40001/plaso/storage/sqlite/merg... plaso/storage/sqlite/merge_reader.py:194: """Prepars for the next container type. On 2018/04/07 13:24:17, onager wrote: > Prepars +Prepares Done.
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.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:166: attribute containers are merged in the correct order. This still doesn't make sense to me - why is alphabetical the "correct" order? What makes alphabetical "correct", as opposed to any other ordering? Please update the docstring to make this clear.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:166: attribute containers are merged in the correct order. where do you read "alphabetical" or infer that from? on the contrary the order is as defined in _CONTAINER_TYPES
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:32: _CONTAINER_TYPES = ( Please add a comment that the order here is significant https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:166: attribute containers are merged in the correct order. On 2018/05/01 08:39:23, Joachim Metz wrote: > where do you read "alphabetical" or infer that from? on the contrary the order > is as defined in _CONTAINER_TYPES OK, I think I got a little confused here, and couldn't understand what this docstring was trying to say. Is this paragraph here to indicate that the ordering of the container types returned here must be preserved? There's also no comment on the _CONTAINER_TYPES definition that indicates that ordering is important.
Sign in to reply to this message.
https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... File plaso/storage/sqlite/merge_reader.py (right): https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:32: _CONTAINER_TYPES = ( On 2018/05/01 08:46:55, onager wrote: > Please add a comment that the order here is significant Done. As discussed this does not address the initial request to update the docstring. https://codereview.appspot.com/340610043/diff/100001/plaso/storage/sqlite/mer... plaso/storage/sqlite/merge_reader.py:166: attribute containers are merged in the correct order. On 2018/05/01 08:46:55, onager wrote: > On 2018/05/01 08:39:23, Joachim Metz wrote: > > where do you read "alphabetical" or infer that from? on the contrary the order > > is as defined in _CONTAINER_TYPES > > OK, I think I got a little confused here, and couldn't understand what this > docstring was trying to say. Is this paragraph here to indicate that the > ordering of the container types returned here must be preserved? There's also no > comment on the _CONTAINER_TYPES definition that indicates that ordering is > important. Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
On 2018/05/01 09:30:28, Joachim Metz wrote: > Code updated. I don't think we're making progress here, so LGTM and I'll send a follow up
Sign in to reply to this message.
as indicated per chat, it is unclear to me what you mean.
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 dbmerge
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
