Skip to content
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

Merged
merged 5 commits into from
Sep 26, 2023
Merged

Add Dissect Processor #3363

merged 5 commits into from
Sep 26, 2023

Conversation

oeyh
Copy link
Collaborator

@oeyh oeyh commented Sep 19, 2023

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

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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.

vishalboin and others added 3 commits September 19, 2023 11:29
Signed-off-by: Vishal Boinapalli <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class FieldHelper {
Copy link
Member

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.

Copy link
Collaborator Author

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 {
Copy link
Member

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

Copy link
Collaborator Author

@oeyh oeyh Sep 21, 2023

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]>
Copy link
Collaborator

@kkondaka kkondaka left a 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

@oeyh oeyh merged commit fee636c into opensearch-project:main Sep 26, 2023
45 of 47 checks passed
@oeyh oeyh mentioned this pull request Sep 26, 2023
4 tasks
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.

Dissect processor
4 participants