You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/04/22 18:58:16 UTC
[GitHub] [nifi] markobean opened a new pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
markobean opened a new pull request #4225:
URL: https://github.com/apache/nifi/pull/4225
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Updates ComponentLog interface to accept Object varargs simplifying logger statements in the code. It is no longer required to create a new Object array; just pass the substitution values directly.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
- [X] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on both JDK 8 and JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-755422595
OK got this merged to `main`. Thanks again @markobean!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 closed pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markap14 closed pull request #4225:
URL: https://github.com/apache/nifi/pull/4225
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-743432494
I was wondering the same thing @joewitt - the change from Object[] to vararg is backward compatible code-wise. I.e., code that was written against a Object[] will still compile against a vararg. But if it was already compiled against Object[], will it run okay when the method is changed varaarg? That needs to be verified before we merge.
This could be tested by building this branch, and then copying in a processor from 1.12.1, for instance, and verifying that the logging is still working properly. If so, then I'm good with the changes.
Regarding the unit test for ParseEvtx: I would just delete the assertion all together. It's testing that something got logged at a warn level. Anything. As long as that log message was the only one and it had some arguments. That's a very odd set of constraints to put on the log message. I would also avoid the change here to an explicit log message. IMO. it is always a bad practice to write unit tests that make assertions against human-readable text. I.e., log message, Exception messages, etc. Those can change at any time in order to make the message more clear, and doing so should never result in a unit test failure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-755391782
Thanks @markobean. Will merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] MikeThomsen commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-648355261
@joewitt @markap14 @alopresto do you concur with my take? If so, I'll close. If not, I'm +1 to merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-648433765
The API is not violated. As an example, the previously required array usage of "new Object { foo, bar }" is still valid and no code changes are required. However, it the new varargs form is _also_ allowed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-744693725
I tested this by building nifi in the feature branch, and installing (1.13.0-SNAPSHOT.) I also placed the 1.12.1 version of two NARs in the lib directory: nifi-standard-nar, nifi-update-attribute-nar. In confirmed that identical log messages were issued by both versions of SplitText and UpdateAttribute processors. I performed this test in both Java 8 and 11.
Also, the odd log statement verification in ParseEvtxTest was removed completely.
I do not see any remaining issues. Please merge.
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-747155013
Looking for a committer for this one. @markap14, any remaining issues? If not, please merge. Appreciate it!
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] joewitt commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
joewitt commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-648445181
I dont quite follow the change to the test class. MOving from an Object[] comparison to any string seems ok but the added isA(flowFile.class) I dont follow.
The change in the API from object[] to varargs is probably ok but I am not positive it is binary compatible where one has an older API but a newer client/usage of it. Have we verified that? Lastly the Object[] was likely chosen to be consistent with the API of some logger out there. We might not want to deviate.
@markap14 Can you take a look and share your thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #4225: NIFI-7387: ComponentLog accepts Object varargs instead of array
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #4225:
URL: https://github.com/apache/nifi/pull/4225#issuecomment-648511269
The isA(flowFile.class) was in the original code and not added as part of this PR. It's a mockito thing which I'm not quite following either. The point of the verify() call in the unit test appears to be to ensure a warning message was logged. The warning message is issued by the processor in this line:
logger.warn("Trying to parse file without .evtx extension {} from flowfile {}", new Object[]{basename, flowFile});
The existing update per this PR retaining the "isA(flowFile.class)" does pass, but might not be doing what is required - or at least not as explicit as the unit test could be. I think this update to the unit test would be more appropriate:
verify(componentLog).warn("Trying to parse file without .evtx extension {} from flowfile {}", basename, flowFile);
If you concur, I'll update the PR code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org