[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2294 @MikeThomsen Thank you for the quick updates! All LGTM now, +1. Merging to master. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @ijokarumawak All of your feedback should now be addressed. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @ijokarumawak I think all of your concerns have been addressed now. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @ijokarumawak @bbende The user who posted on nifi-dev asking for a copy of this confirmed that he's using it in production and is happy with it. If you have any time, could you take a quick look and see if you think it's ready for merge? ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2294 LGTM ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 That restart.index change is in place. I put a unit test in there that tests it out and it works just fine. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @mgaido91 That loop, incidentally, is being eliminated now because I decided to follow the same practice that was used in PutHBaseRecord where on failure it adds "restart.index" which is roughly where in the loop it failed. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2294 thanks @MikeThomsen. Actually I was referring at [this comment](https://github.com/apache/nifi/pull/2294#discussion_r160140592). I think that is my last comment. Overall it LGTM. On the batch size, I think that the committers who will have to review this before merging can express their opinion. Your opinion makes sense, but it involves a consistency issue: we need a tradeoff and I think only committers can decide what to do. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @mgaido91 I added the charset. I can't believe I missed that... WRT the batching, I stand by my opinion that we need to have a sane default there because there needs to be a way to ensure someone doesn't accidentally (or on purpose) send an operation that is too big to HBase at once. To me this is not a theoretical issue because I ran into something like this with PutHBaseRecord doing genomic data ingestion w/ NiFi. The data set would generate easily 10B, if not 20-25B tiny (like few dozen byte) writes. I had to really scale back the size of each record set I was sending to PutHBaseRecord because it was easily to generate so many Puts that it would hammer a region offline unexpectedly. I'm not a HBase expert by any means, but it seems like a recipe for trouble based on my experience with putting a lot of small writes (and Delete objects are tiny writes). ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2294 @MikeThomsen sorry may you please address the last comment? And add the charset parameter? Thanks. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @mgaido91 I'll make the UTF-8 charset a configuration parameter. WRT the batch issue, my preference for providing a batch option with a sane default size comes from seeing less than optimal things involving how much work is sent in a single batch. Trying to be polite on that point, but I think you can get an idea of what I mean... ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 Updated according to the feedback and all tests pass now. ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user joewitt commented on the issue: https://github.com/apache/nifi/pull/2294 Looks like there is some review feedback and Travis-CI fails due to checkstyle issues. Can you please address? ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2294 @MikeThomsen there are validation errors due to unused imports. May you please fix them? ``` [[1;33mWARNING[m] src/main/java/org/apache/nifi/hbase/DeleteHBaseRow.java:[31,8] (imports) UnusedImports: Unused import - java.io.IOException. [[1;33mWARNING[m] src/main/java/org/apache/nifi/hbase/DeleteHBaseRow.java:[33,8] (imports) UnusedImports: Unused import - java.util.HashMap. [[1;33mWARNING[m] src/main/java/org/apache/nifi/hbase/DeleteHBaseRow.java:[35,8] (imports) UnusedImports: Unused import - java.util.Map. ``` ---
[GitHub] nifi issue #2294: NIFI-3538 Added DeleteHBaseRow
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2294 @bbende @ijokarumawak If either of you get a chance, could you take a look at this? I'm working on a big change to the HBase capabilities that would add visibility label support and would like to be able to deliver that w/ delete functionality built in to NiFi users :) ---
