[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16280846#comment-16280846
]
Hudson commented on HBASE-19417:
FAILURE: Integrated in Jenkins build HBase-Trunk_matrix #4180 (See
[https://builds.apache.org/job/HBase-Trunk_matrix/4180/])
HBASE-19417 Remove boolean return value from postBulkLoadHFile hook (tedyu: rev
27ed4d8add20c5424943eee54acff266567e765a)
* (edit)
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
* (edit)
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupObserver.java
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279683#comment-16279683
]
Ted Yu commented on HBASE-19417:
You can see the two test failures here:
https://builds.apache.org/job/HBase-TRUNK_matrix/4176/testReport/
Seems to be related to HBASE-19323
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279680#comment-16279680
]
Ted Yu commented on HBASE-19417:
There was no occurrence of LoadIncrementalHFiles test in the latest build (as
of now):
https://builds.apache.org/job/HBASE-Flaky-Tests/23784/consoleFull
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279676#comment-16279676
]
Appy commented on HBASE-19417:
--
*LoadIncrementalHFiles (related to this change) are not in flaky list though -
https://builds.apache.org/job/HBase-Find-Flaky-Tests/lastSuccessfulBuild/artifact/dashboard.html.
So i'd request you to keep an eye on the flaky list over next few days, in case
they show up after commit.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279624#comment-16279624
]
Ted Yu commented on HBASE-19417:
Ran failed tests locally which passed.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279617#comment-16279617
]
Hadoop QA commented on HBASE-19417:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
10s{color} | {color:blue} Docker mode activated. {color} |
| {color:blue}0{color} | {color:blue} patch {color} | {color:blue} 0m
2s{color} | {color:blue} The patch file was not named according to hbase's
naming conventions. Please see
https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for
instructions. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:blue}0{color} | {color:blue} findbugs {color} | {color:blue} 0m
0s{color} | {color:blue} Findbugs executables are not available. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s{color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} master Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
23s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 4m
43s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
4s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
22s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} shadedjars {color} | {color:green} 6m
18s{color} | {color:green} branch has no errors when building our shaded
downstream artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
43s{color} | {color:green} master passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
14s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:red}-1{color} | {color:red} mvninstall {color} | {color:red} 4m
19s{color} | {color:red} root in the patch failed. {color} |
| {color:red}-1{color} | {color:red} compile {color} | {color:red} 0m
19s{color} | {color:red} hbase-backup in the patch failed. {color} |
| {color:red}-1{color} | {color:red} javac {color} | {color:red} 0m 19s{color}
| {color:red} hbase-backup in the patch failed. {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
26s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedjars {color} | {color:green} 5m
48s{color} | {color:green} patch has no errors when building our shaded
downstream artifacts. {color} |
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 10m
27s{color} | {color:red} The patch causes 15 errors with Hadoop v2.6.1. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 15m
16s{color} | {color:red} The patch causes 15 errors with Hadoop v2.6.2. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 20m
13s{color} | {color:red} The patch causes 15 errors with Hadoop v2.6.3. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 25m
7s{color} | {color:red} The patch causes 15 errors with Hadoop v2.6.4. {color} |
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 30m
1s{color} | {color:red} The patch causes 15 errors with Hadoop v2.6.5. {color} |
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 34m
58s{color} | {color:red} The patch causes 15 errors with Hadoop v2.7.1. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 39m
57s{color} | {color:red} The patch causes 15 errors with Hadoop v2.7.2. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 44m
57s{color} | {color:red} The patch causes 15 errors with Hadoop v2.7.3. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 49m
54s{color} | {color:red} The patch causes 15 errors with Hadoop v2.7.4. {color}
|
| {color:red}-1{color} | {color:red} hadoopcheck {color} | {color:red} 55m
2s{color} | {color:red} The patch causes 15 errors with Hadoop
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279606#comment-16279606
]
Hadoop QA commented on HBASE-19417:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 2m
21s{color} | {color:blue} Docker mode activated. {color} |
| {color:blue}0{color} | {color:blue} patch {color} | {color:blue} 0m
1s{color} | {color:blue} The patch file was not named according to hbase's
naming conventions. Please see
https://yetus.apache.org/documentation/0.6.0/precommit-patchnames for
instructions. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:blue}0{color} | {color:blue} findbugs {color} | {color:blue} 0m
0s{color} | {color:blue} Findbugs executables are not available. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s{color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} master Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
25s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 4m
11s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
54s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
6s{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} shadedjars {color} | {color:green} 5m
23s{color} | {color:green} branch has no errors when building our shaded
downstream artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
35s{color} | {color:green} master passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
11s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 4m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
53s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
53s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
4s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedjars {color} | {color:green} 4m
18s{color} | {color:green} patch has no errors when building our shaded
downstream artifacts. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
46m 53s{color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 2.7.4 or 3.0.0-alpha4. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
39s{color} | {color:green} the patch passed {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 97m 7s{color}
| {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 11m
32s{color} | {color:green} hbase-backup in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
26s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black}177m 8s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests | hadoop.hbase.tool.TestSecureLoadIncrementalHFiles |
| | hadoop.hbase.namespace.TestNamespaceAuditor |
| | hadoop.hbase.regionserver.TestHRegionServerBulkLoadWithOldClient |
| | hadoop.hbase.filter.TestFilterListOnMini |
| | hadoop.hbase.tool.TestLoadIncrementalHFiles |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:eee3b01 |
| JIRA Issue | HBASE-19417 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12900778/19417.v8.txt |
| Optional Tests | asflicense javac javadoc
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279468#comment-16279468
]
Appy commented on HBASE-19417:
--
+1
Great stuff.
bq. It turns out that SecureBulkLoadManager.java has the calls to pre / post
hooks. So in patch v8 I narrowed the calls in RSRpcServices.
oh! cool.
{quote}
bq. CPs in 2.0 are radically different than 1.x
Now I know
{quote}
Oh yeah, it's like any old CP will very very likely not work with 2.0. Will
need a recompile. Base*Observer(s) are gone, many methods have changed, and IA
of many classes changed.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt, 19417.v8.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279422#comment-16279422
]
Ted Yu commented on HBASE-19417:
It turns out that SecureBulkLoadManager.java has the calls to pre / post hooks.
So in patch v8 I narrowed the calls in RSRpcServices.
bq. CPs in 2.0 are radically different than 1.x
Now I know :-)
Addressed the comments in previous round.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279398#comment-16279398
]
Appy commented on HBASE-19417:
--
The reason it should be removed is: with current set of two params - map and
hasLoaded- suggests 4 possible combinations. notNull true; notnull false; null
true; null false. But only two can ever happen: null false, notnull true
That's why extra param is not just redundant, but keeping it suggests all the
possibilities which are never there, removing it makes things simpler.
Javadoc of map param should specify significance of null value anyways, and
that'll be enough.
CPs in 2.0 are radically different than 1.x. A lot has changed. It's fine to
change method definitions.
bq. Remove loaded variable in RSRpcServices#bulkLoadHFile
nits. your choice. i suggested it because it'll be used in only one place after
method refactor, and can have directly used map!= null comparison.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279339#comment-16279339
]
Ted Yu commented on HBASE-19417:
bq. You can change postBulkLoadHFile() to get rid of hasLoaded variable
The variable is used by BackupObserver.
bq. since map variable conveys that information
loaded variable is used afterwards:
{code}
builder.setLoaded(loaded);
{code}
My thinking was:
1. coprocessor hook doesn't need to use boolean expression to infer whether
bulk load has happened
2. the loaded variable is readily available
3. postBulkLoadHFile in branch-1.4 has loaded variable as well
what do you think ?
bq. Remove loaded variable in RSRpcServices#bulkLoadHFile
Answered above.
Will address the last point depending on your feedback.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-19417) Remove boolean return value from postBulkLoadHFile hook
[
https://issues.apache.org/jira/browse/HBASE-19417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16279320#comment-16279320
]
Appy commented on HBASE-19417:
--
You can change postBulkLoadHFile() to get rid of hasLoaded variable, since map
variable conveys that information. Update javadoc for map param.
Remove {{loaded}} variable in RSRpcServices#bulkLoadHFile.
You don't need last two returns in BackupObserver#postBulkLoadHFile
fyi: [~vladmir] [~elserj] BackupObserver will need some changes after this.
Even before this, it wasn't really cancelling bulkload by returning false since
files were already loaded.
> Remove boolean return value from postBulkLoadHFile hook
> ---
>
> Key: HBASE-19417
> URL: https://issues.apache.org/jira/browse/HBASE-19417
> Project: HBase
> Issue Type: Bug
>Reporter: Appy
>Assignee: Ted Yu
> Attachments: 19417.v1.txt, 19417.v2.txt, 19417.v3.txt, 19417.v4.txt,
> 19417.v5.txt, 19417.v6.txt, 19417.v7.txt
>
>
> See the discussion at the tail of HBASE-17123 where Appy pointed out that the
> override of loaded should be placed inside else block:
> {code}
> } else {
> // secure bulk load
> map = regionServer.secureBulkLoadManager.secureBulkLoadHFiles(region,
> request);
> }
> BulkLoadHFileResponse.Builder builder =
> BulkLoadHFileResponse.newBuilder();
> if (map != null) {
> loaded = true;
> }
> {code}
> This issue is to address the review comment.
> After several review iterations, here are the changes:
> * Return value of boolean for postBulkLoadHFile() hook are changed to void.
> * Coprocessor hooks (pre and post) are added for the scenario where bulk load
> manager is used.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
