Skip to content
This repository was archived by the owner on Aug 20, 2025. It is now read-only.

Conversation

@mmiklavc
Copy link
Contributor

https://issues.apache.org/jira/browse/METRON-155

Have not run the Vagrant deploy yet, but this is big PR so i wanted to get this reviewed asap.

  • Added PCAP filter by query.
  • Use Java's Stream, Function, and Predicate impls.
  • Added QueryPcapFilterTest
  • Added tests in REST api for new query option
  • Added integration tests to PCAP topology
  • Fixed bug in QueryParser - handle empty string as pass-through impl like WHERE TRUE

/*
* (non-Javadoc)
*
* @see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the javadocs here to reflect the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like older leftover auto-generated javadoc - I'll revise the packaging and make a proper javadoc for the methods in that class.

@cestella
Copy link
Member

On the whole, this is great. Definitely a great feature and an impressive 2nd PR. Thanks for the contribution!

Please make sure you didn't inadvertently regress the existing pcap functionality on the full-dev-vagrant image by following the testing plan in the original PR to move PCAP to hbase here

@mmiklavc
Copy link
Contributor Author

Ok, made a number of changes for review. Thanks for the feedback! Should have the remaining fixes/improvements avail soon and will also test per comments on full-dev-vagrant.

@nickwallen
Copy link
Contributor

For some reason, I cannot comment on the original JIRA, so I will comment here.

I think using a custom created language for this purpose is going to be confusing for our users. Most network tools support what is called Berkely Packet Filter (BPF) syntax. The user community is going to be fairly familiar with this syntax. I would suggest that we use BPF syntax for this purpose.

@cestella
Copy link
Member

@nickwallen Agreed we should support BPF. This PR makes the filter pluggable and we already have the query language. We can have a follow-on PR for BPF support IMO.

@nickwallen
Copy link
Contributor

I think that makes sense, especially since this creates the 'hook' for later contributions of BPF.

The only downside is that we don't want to confuse users by having 8 ways to do things. But I don't think that will be a problem as long as we eventually get the BPF functionality.

…apper and enhance filtering to use short-circuit Stream function.
@mmiklavc
Copy link
Contributor Author

fyi, I was able to confirm no regressions in the existing PcapJob query via the instructions located here - #93

Note: I had to up map/reduce heap to 1GB and container mem to 1.2GB to avoid OOM errors.

@cestella
Copy link
Member

+1, got my vote.

@dlyle65535
Copy link
Contributor

+1 worked in EC2.

@asfgit asfgit closed this in 3803df2 May 19, 2016
asfgit pushed a commit that referenced this pull request Jun 24, 2016
asfgit pushed a commit that referenced this pull request Jun 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants