You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by ijokarumawak <gi...@git.apache.org> on 2017/10/30 02:19:09 UTC
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
GitHub user ijokarumawak opened a pull request:
https://github.com/apache/nifi/pull/2237
NIFI-4543: Improve HBase processors provenance transit URL
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?
- [ ] 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/ijokarumawak/nifi nifi-4543
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2237.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 #2237
----
commit eb8f114df2515faa3d0fc110a70256e05c95f55f
Author: Koji Kawamura <ij...@apache.org>
Date: 2017-10-30T02:03:18Z
NIFI-4543: Improve HBase processors provenance transit URL
----
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2237#discussion_r147990929
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-hbase_1_1_2-client-service-bundle/nifi-hbase_1_1_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_1_1_2_ClientService.java ---
@@ -535,4 +539,16 @@ public Configuration getConfiguration() {
public byte[] toBytesBinary(String s) {
return Bytes.toBytesBinary(s);
}
+
+ @Override
+ public String toTransitUri(String tableName, String rowKey) {
+ if (connection == null) {
+ logger.warn("Connection has not been established, could not create a transit URI. Returning null.");
+ return null;
+ }
+ final Configuration configuration = connection.getConfiguration();
+ // Remove white spaces.
+ final String zkQuorum = configuration.get(HBASE_CONF_ZK_QUORUM).replaceAll("\\s", "");
+ return "hbase://" + zkQuorum + "/" + tableName + (rowKey != null && !rowKey.isEmpty() ? "/" + rowKey : "");
--- End diff --
I don't think it does. I am not a HBase expert by any means, but I know that you can manually specify the IP address to the HBase master when configuring a Java client. This URL could also be problematic for people attempting a Thrift connection via Python or another client that side steps ZooKeeper.
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2237#discussion_r147998027
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java ---
@@ -196,4 +196,15 @@
*/
byte[] toBytesBinary(String s);
+ /**
+ * Create a transit URI from the current configuration and the specified table name.
+ * The default implementation just prepend "hbase://" to the table name and row key, i.e. "hbase://tableName/rowKey".
+ * @param tableName The name of a HBase table
+ * @param rowKey The target HBase row key, this can be null or empty string if the operation is not targeted to a specific row
+ * @return a qualified transit URI which can identify a HBase table row in a HBase cluster
+ */
+ default String toTransitUri(String tableName, String rowKey) {
+ return "hbase://" + tableName + (rowKey != null && !rowKey.isEmpty() ? "/" + rowKey : "");
--- End diff --
Are there any scenarios where `rowKey` will be null or empty?
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2237
---
[GitHub] nifi issue #2237: NIFI-4543: Improve HBase processors provenance transit URL
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2237
@MikeThomsen @pvillard31 `getMaster()` worked fine. I think it's better than Zk quorum. Thanks for suggesting that. Pushed another commit to incorporate that. Does it look fine now?
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2237#discussion_r148529922
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java ---
@@ -196,4 +196,15 @@
*/
byte[] toBytesBinary(String s);
+ /**
+ * Create a transit URI from the current configuration and the specified table name.
+ * The default implementation just prepend "hbase://" to the table name and row key, i.e. "hbase://tableName/rowKey".
+ * @param tableName The name of a HBase table
+ * @param rowKey The target HBase row key, this can be null or empty string if the operation is not targeted to a specific row
+ * @return a qualified transit URI which can identify a HBase table row in a HBase cluster
+ */
+ default String toTransitUri(String tableName, String rowKey) {
+ return "hbase://" + tableName + (rowKey != null && !rowKey.isEmpty() ? "/" + rowKey : "");
--- End diff --
Currently probably no scenarios, however if NiFi starts providing operations against HBase tables meaning not with a specific row, such as DDL, Phenix query or passing scan result as Record then there will be situations rowKey is null.
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2237#discussion_r147658468
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-hbase_1_1_2-client-service-bundle/nifi-hbase_1_1_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_1_1_2_ClientService.java ---
@@ -535,4 +539,16 @@ public Configuration getConfiguration() {
public byte[] toBytesBinary(String s) {
return Bytes.toBytesBinary(s);
}
+
+ @Override
+ public String toTransitUri(String tableName, String rowKey) {
+ if (connection == null) {
+ logger.warn("Connection has not been established, could not create a transit URI. Returning null.");
+ return null;
+ }
+ final Configuration configuration = connection.getConfiguration();
+ // Remove white spaces.
+ final String zkQuorum = configuration.get(HBASE_CONF_ZK_QUORUM).replaceAll("\\s", "");
+ return "hbase://" + zkQuorum + "/" + tableName + (rowKey != null && !rowKey.isEmpty() ? "/" + rowKey : "");
--- End diff --
does it make sense to add znode? Two HBase clusters could use the same ZK, right?
(not sure what would be the best way to represent it though, using ``/znode/table`` could be confusing, no?)
---
[GitHub] nifi issue #2237: NIFI-4543: Improve HBase processors provenance transit URL
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2237
+1, ran full build with contrib check and confirmed expected output in events, merging to master, thanks @ijokarumawak and @MikeThomsen
---
[GitHub] nifi pull request #2237: NIFI-4543: Improve HBase processors provenance tran...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2237#discussion_r147999357
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-hbase_1_1_2-client-service-bundle/nifi-hbase_1_1_2-client-service/src/main/java/org/apache/nifi/hbase/HBase_1_1_2_ClientService.java ---
@@ -535,4 +539,16 @@ public Configuration getConfiguration() {
public byte[] toBytesBinary(String s) {
return Bytes.toBytesBinary(s);
}
+
+ @Override
+ public String toTransitUri(String tableName, String rowKey) {
+ if (connection == null) {
+ logger.warn("Connection has not been established, could not create a transit URI. Returning null.");
+ return null;
+ }
+ final Configuration configuration = connection.getConfiguration();
+ // Remove white spaces.
+ final String zkQuorum = configuration.get(HBASE_CONF_ZK_QUORUM).replaceAll("\\s", "");
+ return "hbase://" + zkQuorum + "/" + tableName + (rowKey != null && !rowKey.isEmpty() ? "/" + rowKey : "");
--- End diff --
Try this for the master IP:
`connection.getAdmin().getClusterStatus().getMaster()`
That returns a `ServerName` instance which looks like it has everything you need.
[https://hbase.apache.org/apidocs/org/apache/hadoop/hbase/ServerName.html](ServerName javadoc)
---
[GitHub] nifi issue #2237: NIFI-4543: Improve HBase processors provenance transit URL
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2237
@pvillard31 @MikeThomsen Thanks for the comments! `connection.getAdmin().getClusterStatus().getMaster()` looks promising. I will test that approach and update PR.
---
[GitHub] nifi issue #2237: NIFI-4543: Improve HBase processors provenance transit URL
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2237
@ijokarumawak Looks good to me.
---