[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-08-01 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 Thanks to everyone for the feedback and comments! ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-08-01 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 This has been merged into the feature branch. Can you close it @tiborm? ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-08-01 Thread mmiklavc
Github user mmiklavc commented on the issue: https://github.com/apache/metron/pull/1103 Looks like all comments have been addressed, reviewers have been able to test successfully, and any remaining follow-on work has been added to the feature branch Jira/Epic. I'm +1 by inspection. Ni

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-08-01 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 @sardell Thanks for adding the tickets. +1, @tiborm Thanks for your patience during the whole review process. I'm looking forward to getting the ball rolling on the rest of the contribut

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-08-01 Thread sardell
Github user sardell commented on the issue: https://github.com/apache/metron/pull/1103 @justinleet Here are the tickets for the follow-on items: * Input validation: https://issues.apache.org/jira/browse/METRON-1712 * Kill button for in progress jobs: https://issues.apache.

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-31 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 @sardell I'm fine with this stuff being follow on. Can we make sure we have tickets linked to https://issues.apache.org/jira/browse/METRON-1554? I believe the follow-ons are -

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-30 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 Several people have given feedback on this PR. What do you think @mmiklavc, @justinleet and @cestella? Is this good enough for a first pass at the UI? ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-27 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 Given that unit tests will be addressed in a forthcoming PR, this is good enough for me as an initial UI. +1 pending other commenters are satisfied. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-27 Thread sardell
Github user sardell commented on the issue: https://github.com/apache/metron/pull/1103 @justinleet In response to your UI-related questions: > Better input validation would be nice. I can currently enter negative ports (which don't match anything) or ports beyond max int (whic

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-27 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 As part of the latest commits I removed commented code blocks, and fixed the variable naming issues in pcap-packet-line.component.ts. This PR and the followup ones are updated by the latest change

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-27 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 @mmiklavc Thanks for the comment! I extended the JIRA ticket with user story like test scenarios. Also added a short description of to the PR description about how to spin up a full dev with pcap

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-26 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 @mmiklavc The list of the sequential PR's in order are the following: METRON-1671: Initial PCAP UI https://github.com/apache/metron/pull/1103 METRON-1662: Adding download button

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-26 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 @cestella We added the license headers for all new files. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-25 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 @mmiklavc We had to update a package-lock.json file because of the original one contained a package collision. npm ci command just failed on that. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-25 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 In the screenshots you provide, the timestamp field contains both the epoch timestamp and the date. I'd have to dig in, but based on PDML files I've seen, it's displaying both the "show" and the

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-24 Thread mmiklavc
Github user mmiklavc commented on the issue: https://github.com/apache/metron/pull/1103 @tiborm - I see a lot of recent changes to dependency versions in the package lock file - is that intentional? Just a short comment about what it's for would be helpful. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-24 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 @mmiklavc The job manager is asynchronous in that it accepts a Finalizer. We have the polling loop in place but we would need to refactor the job manager to expose a callback function for getStat

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread mmiklavc
Github user mmiklavc commented on the issue: https://github.com/apache/metron/pull/1103 > I'm not sure the Hadoop Job object is asynchronous anyways. @merrimanr If I'm right on the context of the Q and A here, the PcapJob around the underlying MR job handles it async or sync. For

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 @merrimanr Up front I want to say I agree async is follow-on. I think polling is fine for right now, but there are potential scaling concerns and I wanted to get a sense of the reasoning. In pr

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 @justinleet I disagree with "Polling seems pretty avoidable". I feel like that would be significant work and I'm not sure the Hadoop Job object is asynchronous anyways. We would have to poll som

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 The other feedback I have is that the unit tests need some work. For example, when I updated `pcap.request.ts` but hadn't updated `pcap-filters.component.html`, the unit tests still pass. That t

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 Can I also get some insight into why the job status REST call and UI usage isn't an asynchronous call? Polling seems pretty avoidable here. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 I have a couple comments after spinning up #1122 (which should have all these changes, I believe). - Better input validation would be nice. I can currently enter negative ports (which

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread mmiklavc
Github user mmiklavc commented on the issue: https://github.com/apache/metron/pull/1103 > Have you included steps to reproduce the behavior or problem that is being changed or addressed? > > Have you included steps or a guide to how the change may be verified and tested man

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-23 Thread merrimanr
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/1103 I tested this in full dev and the fixed query request is incorrect. Here's what is being sent: ``` { "startTime": 0, "endTime": 15, "srcIp": "", "sr

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-20 Thread cestella
Github user cestella commented on the issue: https://github.com/apache/metron/pull/1103 I have submitted a PR to up the rat plugin version and have fixed it in master #1126, let's make sure to add license headers here for new files added in this branch though. ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-20 Thread justinleet
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/1103 There's a bug report that rat treats .ts files as binary and ignores them. https://issues.apache.org/jira/browse/RAT-234 ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-20 Thread mmiklavc
Github user mmiklavc commented on the issue: https://github.com/apache/metron/pull/1103 @tiborm @sardell Can one of you list out the UI PR's and indicate the order in which they should be tested and subsequently merged into the feature branch? Since this is the initial major ticket, y

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-18 Thread james-sirota
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/1103 I reviewed the UI. Everything comes up fine and looks good. One assumption I am making is that protocol is numeric because the protocol field will not allow me to enter a character. +1 ---

[GitHub] metron issue #1103: METRON-1671: Initial PCAP UI

2018-07-17 Thread tiborm
Github user tiborm commented on the issue: https://github.com/apache/metron/pull/1103 @mmiklavc The 10k+ line spec file is a working unit test which contains mock data as well. Unfortunately, some other unit tests failing in this PR. We already to a separate ticket for fixing and