Skip to content

updated packetmail to use csv.#98

Merged
alexcpsec merged 3 commits intomlsecproject:masterfrom
btv:master
Dec 27, 2014
Merged

updated packetmail to use csv.#98
alexcpsec merged 3 commits intomlsecproject:masterfrom
btv:master

Conversation

@btv
Copy link
Copy Markdown
Contributor

@btv btv commented Dec 23, 2014

This is less of an actual MR and more about trying to start a discussion about migrating the input parsing to being handled by the csv module instead of being done by hand. Let me know what you think and I can modify the rest of the parseing functiont to do the same.

Testing for this was:

  • call reaper
  • call old thresher code to produce harvest.json, renamed to harvest.json.old
  • change branch to new thresher code
  • called thresher again to produce a new harvest.json file
  • ran diff -q between the two files, none were found.

Happy Holidays!

@alexcpsec
Copy link
Copy Markdown
Member

Hey! Thanks for this.

I think this makes a lot of sense, and the new method you propose makes it prone to less errors in the longer run. Please make the changes to the other parsing functions as you see fit. No rush, take your time. :)

In the meantime, I will forward you the CLA.

Happy Holidays!

@alexcpsec
Copy link
Copy Markdown
Member

Thanks, Bryce. LMK if you would like me to test and merge this separately or if you prefer to have it all done first.

@btv
Copy link
Copy Markdown
Contributor Author

btv commented Dec 27, 2014

Hey Alex,

Happy Holidays and sorry for the delay in my reply. I'm happy for you to
test and merge if you want. I did the best I could to test my change,
which I documented in my original MR request. However, if you know a
better way to test would you please share that with me?I definitely want
to make sure I keep things the same as I do the "remodel".

Warm regards,
Bryce

On 12/23/2014 10:25 AM, Alex Pinto wrote:

Thanks, Bryce. LMK if you would like me to test and merge this
separately or if you prefer to have it all done first.


Reply to this email directly or view it on GitHub
#98 (comment).

@alexcpsec
Copy link
Copy Markdown
Member

No worries, Bryce. Happy holidays!
The test you suggested was ok! What I meant was if you wanted to get a go at the remodel part of the rest of the functions before I merged this.

I'll go ahead and merge this one, and then you can carry on with the changes as you see fit.

@alexcpsec
Copy link
Copy Markdown
Member

Actually, you should compare crop.json before and after. Just ran a quick test and packetmail is not parsing anything with the new code. Could you check it again, please?

Pulled the call for the current date out of the for loops. Should
improve performance.
@btv
Copy link
Copy Markdown
Contributor Author

btv commented Dec 27, 2014

Hey Alex,

I figured out that I was testing against the wrong file. I've verified
that my change doesn't actually work so I'm in the process of fixing it.
I'll let you know when I have it working.

Sorry about that!

On 12/26/2014 10:39 PM, Alex Pinto wrote:

Actually, you should compare |crop.json| before and after. Just ran a
quick test and packetmail is not parsing anything with the new code.
Could you check it again, please?


Reply to this email directly or view it on GitHub
#98 (comment).

response is of a type where csv.reader needs the splitlines method.
@btv
Copy link
Copy Markdown
Contributor Author

btv commented Dec 27, 2014

Fixed! Like the commit message says, I need to use the splitlines method on response. I also pulled out the today functions from within the loops. These changes improved the run time of thresher from 14.5 seconds to 10.5 seconds on my machine.

@alexcpsec
Copy link
Copy Markdown
Member

Nice catch. There should be several small optimizations like this on the code. Tested this and merging soon.

@alexcpsec alexcpsec merged commit 31aa1e3 into mlsecproject:master Dec 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants