You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by bdesert <gi...@git.apache.org> on 2018/03/09 17:19:13 UTC
[GitHub] nifi pull request #2527: FetchHBaseRow - log level and displayName
GitHub user bdesert opened a pull request:
https://github.com/apache/nifi/pull/2527
FetchHBaseRow - log level and displayName
update log level for "not found" to DEBUG instead of ERROR, and added displayName to all property descriptors.
-----
Thank you for submitting a contribution to Apache NiFi.
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?
### 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?
- [ ] 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?
- [x] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bdesert/nifi NIFI-4953-FetchHBaseRaw-logs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2527.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2527
----
commit 827759a7fc32bb421e8e0011a2815201f60e1086
Author: Ed <ed...@...>
Date: 2018-03-09T17:06:34Z
FetchHBaseRow - log level and displayName
update log level for "not found" to DEBUG instead of ERROR, and added displayName to all property descriptors
----
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2527
What sort of regression was this supposed to address?
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by bdesert <gi...@git.apache.org>.
Github user bdesert commented on the issue:
https://github.com/apache/nifi/pull/2527
@pvillard31 , @bbende In case we want to have bulletin to be generated, we can change "Bulletin Level" of the component to "DEBUG". But then bulletin will be generated even if rowkey is found. I would rather remove lines 276-278, to have clean debug level bulletin for "not found" only.
Your thoughts?
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/2527
I think when I initially implemented this processor I was assuming someone would be fetching rows that were expected to be there, so if it wasn't there then it would be more an error scenario.
However, looking back on it now... since we do have the explicit not found relationship, it is going to be obvious when something is not found, and we can use LogMessage as Pierre mentioned if we need to generate a bulletin there. So I'm ok with changing it to debug.
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by bdesert <gi...@git.apache.org>.
Github user bdesert commented on the issue:
https://github.com/apache/nifi/pull/2527
@MikeThomsen , I think since it is already in 1.5 and ppl started using it, it could be a case, when implementation can use rest APIs to work with these processors. And since we are changing "name" property (which cannot be controlled by user), it can create a problem with those eixsting scripts. Also, name is a part of flow.xml.gz, that will also impact existing implementations and make regression impact. So I had to agree to Pierre comment about regression.
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2527
Thought a bit more about this one and I wondered if someone could be in a situation where they really want a bulletin to be generated when the row is not found. But since the flow file will be routed to a specific relationship (REL_NOT_FOUND), they can actually generate a bulletin using another processor (like LogMessage) for this relationship. Another option would be to let the user choose the behavior with a property but it sounds a bit overcomplicated... What do you think @bbende? (you created this processor, I'm curious to have your opinion)
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2527
Hi @bdesert - unfortunately, since this processor has been already released with NiFi 1.5.0, we cannot change the name of the properties in a minor release as it would break the existing workflows. That's why we encourage the use of both name() and displayName() when a new processor is created so that we can change the displayName() value without impacting existing workflows. I think you can just drop the changes for properties name and just leave the log level change.
---
[GitHub] nifi pull request #2527: FetchHBaseRow - log level and displayName
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2527
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2527
@bdesert When I read the commit, I wasn't sure what regression it was actually fixing.
---
[GitHub] nifi issue #2527: FetchHBaseRow - log level and displayName
Posted by bdesert <gi...@git.apache.org>.
Github user bdesert commented on the issue:
https://github.com/apache/nifi/pull/2527
oh, I see now. so it was about my first commit, when I added "displayName" and changed the "name" as per standard. But after Pierre's comment removed those fixes.
Thanks for taking a look anyway!
---