-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Dissect Processor #3363
Add Dissect Processor #3363
Conversation
Signed-off-by: Vishal Boinapalli <[email protected]>
Signed-off-by: Vishal Boinapalli <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
import java.util.regex.Matcher; | ||
import java.util.regex.Pattern; | ||
|
||
public class FieldHelper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some missing unit tests on this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
|
||
public class Dissector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this class has no unit tests and is being tested indirectly in DissectProcessorTest
. Ideally we would mock this class and have unit tests for it directly, but I won't block the PR on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I separated the tests for Dissector from DissectProcessorTest. DissectProcessorTest
now only tests things after the actual dissection is done - e.g., handle dissect exception, convert type, and put dissected fields into event.
…essor; add delimiter and fieldhelper tests Signed-off-by: Hai Yan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have looked at these changes when Vishal submitted PR#3090
Description
Moved commits from #3090 by @vishalboin, rebased to resolve merge conflicts and made some small tweaks on tests and docs.
Issues Resolved
Resolves #3362
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.