Skip to content

NIFI-10019: SendTrapSNMP works without flowfile, upgraded to JUnit5#6046

Closed
Lehel44 wants to merge 5 commits intoapache:mainfrom
Lehel44:NIFI-10019
Closed

NIFI-10019: SendTrapSNMP works without flowfile, upgraded to JUnit5#6046
Lehel44 wants to merge 5 commits intoapache:mainfrom
Lehel44:NIFI-10019

Conversation

@Lehel44
Copy link
Contributor

@Lehel44 Lehel44 commented May 13, 2022

Summary

  • SendTrapSNMP fix to work without flowfile as well
  • Upgraded tests from JUnit4 to JUnit5

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 8
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@nandorsoma nandorsoma left a comment

Choose a reason for hiding this comment

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

Hey @Lehel44!
Thank you for this contribution! I have tested the feature with ListenTrapSNMP receiving SendTrapSNMP's request. I have inline comments and I also found this bug during testing:

When I set Generic Trap Type to 1 and Specific Trap Type to 3 I get a validation exception, but after that a NullPointerException too. I think we shouldn't be able to start the processor and then we won't get the second exception.

java.lang.IllegalArgumentException: Invalid argument: Generic Trap Type is not [6 - Enterprise Specific] but Specific Trap Type is provided.
        at org.apache.nifi.snmp.configuration.V1TrapConfiguration$Builder.build(V1TrapConfiguration.java:112)
        at org.apache.nifi.snmp.processors.SendTrapSNMP.onTrigger(SendTrapSNMP.java:145)
…
2022-05-20 14:23:37,376 ERROR [Timer-Driven Process Thread-9] o.a.nifi.snmp.processors.SendTrapSNMP SendTrapSNMP[id=e14c1a22-0180-1000-8d49-668bf9378c73] Processing halted: yielding [1 sec]
java.lang.NullPointerException: null
        at org.apache.nifi.controller.repository.StandardProcessSession.getRecord(StandardProcessSession.java:800)
        at org.apache.nifi.controller.repository.StandardProcessSession.validateRecordState(StandardProcessSession.java:3701)
        at org.apache.nifi.controller.repository.StandardProcessSession.penalize(StandardProcessSession.java:2195)

Fully configured processor looked like this when I was testing:
Screen Shot 2022-05-20 at 2 32 19 PM

If I understand correctly Specific Trap Type is only needed when Generic Trap Type is 6. Shouldn't we make that property depends on the other? Also would it make sense to select the values instead of using a text field?

Another things I've found during testing, but they are not part of the PR:

  • If 0 value for port is invalid, then why it is the default value? Why don't we init it with null value or with a standard port if there is any for this protocol?
  • ListenTrapSNMP ran almost a million times, while for example ListenTCP ran 556 times in the same interval.
  • More interesting is that ListenTCP measured correctly the Time for the task, but ListenTrapSNMP not, while as I said both were running for about 5 secs. Please see the screenshots below:
    image
    image

@Lehel44
Copy link
Contributor Author

Lehel44 commented May 23, 2022

@nandorsoma Thank you for the review! I will check the ListenTrapSNMP running times in #6034 as this PR deals only with SendTrapSNMP.
I think SpecificTrapType cannot depend on GenericTrapType value because Expression Language is supported with flowfile scope. Validation happens before we start the processor but when the values are being parsed from flowfile attributes runtime exception will be thrown.

Copy link
Contributor

@nandorsoma nandorsoma left a comment

Choose a reason for hiding this comment

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

Hey @Lehel44!
Thank you for the additional changes. I have one question inline and also responded to you in one of our previous conversation.

@nandorsoma
Copy link
Contributor

Oh, and an extra one: Shouldn't additional commits start with the ticket number?

@Lehel44
Copy link
Contributor Author

Lehel44 commented May 25, 2022

No, it will be squashed in the end.

@tpalfy
Copy link
Contributor

tpalfy commented Jul 18, 2022

LGTM, thanks for your work @Lehel44!
Pushed to main.

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.

3 participants