[jira] [Comment Edited] (HBASE-20691) Storage policy should allow deferring to HDFS

2018-07-04 Thread Yu Li (JIRA)


[ 
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16532310#comment-16532310
 ] 

Yu Li edited comment on HBASE-20691 at 7/4/18 6:27 AM:
---

Pushed into master, branch-2, branch-2.0, branch-2.1, thanks all for review.

The branch-1 code base is different, attaching the patch for branch-1, will 
commit after HadoopQA check.


was (Author: carp84):
Pushed into master, branch-2, branch-2.1, branch-2.2, thanks all for review.

The branch-1 code base is different, attaching the patch for branch-1, will 
commit after HadoopQA check.

> Storage policy should allow deferring to HDFS
> -
>
> Key: HBASE-20691
> URL: https://issues.apache.org/jira/browse/HBASE-20691
> Project: HBase
>  Issue Type: Bug
>  Components: Filesystem Integration, wal
>Affects Versions: 1.5.0, 2.0.0
>Reporter: Sean Busbey
>Assignee: Yu Li
>Priority: Blocker
> Fix For: 2.1.0, 1.5.0
>
> Attachments: HBASE-20691.branch-1.patch, HBASE-20691.patch, 
> HBASE-20691.v2.patch, HBASE-20691.v3.patch, HBASE-20691.v4.patch, 
> HBASE-20691.v5.patch, HBASE-20691.v6.patch, HBASE-20691.v7.patch, 
> HBASE-20691.v8.patch
>
>
> In HBase 1.1 - 1.4 we can defer storage policy decisions to HDFS by using 
> "NONE" as the storage policy in hbase configs.
> As described on this [dev@hbase thread "WAL storage policies and interactions 
> with Hadoop admin 
> tools."|https://lists.apache.org/thread.html/d220726fab4bb4c9e117ecc8f44246402dd97bfc986a57eb2237@%3Cdev.hbase.apache.org%3E]
>  we no longer have that option in 2.0.0 and 1.5.0 (as the branch is now). 
> Additionally, we can't set the policy to HOT in the event that HDFS has 
> changed the policy for a parent directory of our WALs.
> We should put back that ability. Presuming this is done by re-adopting the 
> "NONE" placeholder variable, we need to ensure that value doesn't get passed 
> to HDFS APIs. Since it isn't a valid storage policy attempting to use it will 
> cause a bunch of logging churn (which will be a regression of the problem 
> HBASE-18118 sought to fix).



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Comment Edited] (HBASE-20691) Storage policy should allow deferring to HDFS

2018-06-21 Thread Yu Li (JIRA)


[ 
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16519029#comment-16519029
 ] 

Yu Li edited comment on HBASE-20691 at 6/21/18 7:46 AM:


bq. Right now the code in CommonFSUtils just checks against the default passed 
into the call to setStoragePolicy, not against any constant. That's incorrect
Ok, got your point, it follows this mode since introduced by HBASE-12848. Let 
me remove the {{setStoragePolicy}} method with the *defaultPolicy* parameter 
and directly check against constant in the {{setStoragePolicy}} method with 3 
parameters.

bq. I don't mean in the region server, I mean just here in this test.
Ah I see, the test case here simply tries to prove the HDFS api won't be called 
if we tries to set the storage policy to default, and vice versa. Please check 
the new patch and I think it will be much more clear. Please note that the 
IOException thrown will be caught and *logged as a warning* like below (I guess 
you ignored the UT result I pasted above sir, so allow me to repeat):
{noformat}
2018-06-08 22:59:39,063 WARN  [Time-limited test] util.CommonFSUtils(572): 
Unable to set storagePolicy=HOT for path=non-exist. DEBUG log level might have 
more details.
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.invokeSetStoragePolicy(CommonFSUtils.java:563)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:524)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:484)
at 
org.apache.hadoop.hbase.util.TestFSUtils.verifyNoHDFSApiInvocationForDefaultPolicy(TestFSUtils.java:356)
at 
org.apache.hadoop.hbase.util.TestFSUtils.testSetStoragePolicyDefault(TestFSUtils.java:341)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
Caused by: java.io.IOException: The setStoragePolicy method is invoked 
unexpectedly
at 
org.apache.hadoop.hbase.util.TestFSUtils$AlwaysFailSetStoragePolicyFileSystem.setStoragePolicy(TestFSUtils.java:364)
... 30 more
{noformat}


was (Author: carp84):
bq. Right now the code in CommonFSUtils just checks against the default passed 
into the call to setStoragePolicy, not against any constant. That's incorrect
Ok, got your point, it follows this mode since introduced by HBASE-12848. Let 
me remove the {{setStoragePolicy}} method with the *defaultPolicy* parameter 
and directly check against constant in the {{setStoragePolicy}} method with 3 
parameters.

bq. I don't mean in the region server, I mean just here in this test.
Ah I see, the test case here simply tries to prove the HDFS api won't be called 
if we tries to set the storage policy to default, and vice versa. Please check 
the new patch and I think it will be much more clear. Please note that the 
IOException thrown will be caught and logged as a warning like below (I guess 
you ignored the UT result I pasted above sir, so allow me to repeat):
{noformat}
2018-06-08 22:59:39,063 WARN  [Time-limited test] util.CommonFSUtils(572): 
Unable to set storagePolicy=HOT for path=non-exist. DEBUG log level might have 
more details.
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.invokeSetStoragePolicy(CommonFSUtils.java:563)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:524)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:484)
at 
org.apache.hadoop.hbase.util.TestFSUtils.verifyNoHDFSApiInvocationForDefaultPolicy(TestFSUtils.java:356)
at 
org.apache.hadoop.hbase.util.TestFSUtils.testSetStoragePolicyDefault(TestFSUtils.java:341)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
Caused by: java.io.IOException: The setStoragePolicy method is invoked 
unexpectedly
at 
org.apache.hadoop.hbase.util.TestFSUtils$AlwaysFailSetStoragePolicyFileSystem.setStoragePolicy(TestFSUtils.java:364)
... 30 more
{noformat}

> Storage policy should allow deferring to HDFS
> -
>
> Key: HBASE-20691
> URL: 

[jira] [Comment Edited] (HBASE-20691) Storage policy should allow deferring to HDFS

2018-06-21 Thread Yu Li (JIRA)


[ 
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16519029#comment-16519029
 ] 

Yu Li edited comment on HBASE-20691 at 6/21/18 7:45 AM:


bq. Right now the code in CommonFSUtils just checks against the default passed 
into the call to setStoragePolicy, not against any constant. That's incorrect
Ok, got your point, it follows this mode since introduced by HBASE-12848. Let 
me remove the {{setStoragePolicy}} method with the *defaultPolicy* parameter 
and directly check against constant in the {{setStoragePolicy}} method with 3 
parameters.

bq. I don't mean in the region server, I mean just here in this test.
Ah I see, the test case here simply tries to prove the HDFS api won't be called 
if we tries to set the storage policy to default, and vice versa. Please check 
the new patch and I think it will be much more clear. Please note that the 
IOException thrown will be caught and logged as a warning like below (I guess 
you ignored the UT result I pasted above sir, so allow me to repeat):
{noformat}
2018-06-08 22:59:39,063 WARN  [Time-limited test] util.CommonFSUtils(572): 
Unable to set storagePolicy=HOT for path=non-exist. DEBUG log level might have 
more details.
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.invokeSetStoragePolicy(CommonFSUtils.java:563)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:524)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:484)
at 
org.apache.hadoop.hbase.util.TestFSUtils.verifyNoHDFSApiInvocationForDefaultPolicy(TestFSUtils.java:356)
at 
org.apache.hadoop.hbase.util.TestFSUtils.testSetStoragePolicyDefault(TestFSUtils.java:341)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
Caused by: java.io.IOException: The setStoragePolicy method is invoked 
unexpectedly
at 
org.apache.hadoop.hbase.util.TestFSUtils$AlwaysFailSetStoragePolicyFileSystem.setStoragePolicy(TestFSUtils.java:364)
... 30 more
{noformat}


was (Author: carp84):
bq. Right now the code in CommonFSUtils just checks against the default passed 
into the call to setStoragePolicy, not against any constant. That's incorrect
Ok, got your point, it follows this mode since introduced by HBASE-12848. Let 
me remove the {{setStoragePolicy}} method with the *defaultPolicy* parameter 
and directly check against constant in the {{setStoragePolicy}} method with 3 
parameters.

bq. I don't mean in the region server, I mean just here in this test.
Ah I see, the test case here simply tries to prove the HDFS api won't be called 
if we tries to set the storage policy to default, and vice versa. Please check 
the new patch and I think it will be much more clear

> Storage policy should allow deferring to HDFS
> -
>
> Key: HBASE-20691
> URL: https://issues.apache.org/jira/browse/HBASE-20691
> Project: HBase
>  Issue Type: Bug
>  Components: Filesystem Integration, wal
>Affects Versions: 1.5.0, 2.0.0
>Reporter: Sean Busbey
>Assignee: Yu Li
>Priority: Minor
> Fix For: 2.1.0, 1.5.0
>
> Attachments: HBASE-20691.patch, HBASE-20691.v2.patch, 
> HBASE-20691.v3.patch, HBASE-20691.v4.patch
>
>
> In HBase 1.1 - 1.4 we can defer storage policy decisions to HDFS by using 
> "NONE" as the storage policy in hbase configs.
> As described on this [dev@hbase thread "WAL storage policies and interactions 
> with Hadoop admin 
> tools."|https://lists.apache.org/thread.html/d220726fab4bb4c9e117ecc8f44246402dd97bfc986a57eb2237@%3Cdev.hbase.apache.org%3E]
>  we no longer have that option in 2.0.0 and 1.5.0 (as the branch is now). 
> Additionally, we can't set the policy to HOT in the event that HDFS has 
> changed the policy for a parent directory of our WALs.
> We should put back that ability. Presuming this is done by re-adopting the 
> "NONE" placeholder variable, we need to ensure that value doesn't get passed 
> to HDFS APIs. Since it isn't a valid storage policy attempting to use it will 
> cause a bunch of logging churn (which will be a regression of the problem 
> HBASE-18118 sought to fix).



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Comment Edited] (HBASE-20691) Storage policy should allow deferring to HDFS

2018-06-21 Thread Sean Busbey (JIRA)


[ 
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16518968#comment-16518968
 ] 

Sean Busbey edited comment on HBASE-20691 at 6/21/18 6:15 AM:
--

{quote}
bq. CommonFSUtils should be updated to check against 
DEFER_TO_HDFS_STORAGE_POLICY

It should check against the default storage policy and let through if so. 
Currently the default policy is set to "NONE", and we give it a comprehensive 
constant name DEFER_TO_HDFS_STORAGE_POLICY
{quote}

Right now the code in CommonFSUtils just checks against the default passed into 
the call to {{setStoragePolicy}}, not against any constant. That's incorrect. 
E.g. if someone calls {{CommonFSUtils.setStoragePolicy(fs, conf, 
"get.this.storage.policy.key", "HOT")}} then if "get.this.storage.policy.key" 
isn't set it should a) use the value "HOT" and b) actually pass that value to 
the FileSystem implementation. When checking against a constant, the one it 
should check is DEFER_TO_HDFS_STORAGE_POLICY because changing our default 
policy for WALs shouldn't change which values are passed through to the 
FileSystem instance.

{quote}
bq. the test should be in TestCommonFSUtils

Agree that they should be. Checking commit history, the set-storage related 
test cases were added in TestFSUtils by HBASE-13498 and somehow left there 
during the code refactor in HBASE-18784... How about we open a follow-on JIRA 
to move all set-storage test cases into TestCommonFSUtils after this one?
{quote}

Sure. please link it here.

{quote}
bq. Shouldn't this second invocation have thrown an IOException?

Personally I think it's OK to let it fail silently with some warning log if the 
given policy is invalid or the set policy attempt failed, as the current 
implementation does. Throwing an IOE and cause region fail to open is too much 
IMHO.
{quote}

I don't mean in the region server, I mean just here in this test. the second 
call effectively uses "HOT" as the storage policy. That's a policy that we 
should give to the underlying FileSystem. The call in the test passes 
{{testFs}} as the FileSystem instance, which is an instance of the newly added 
"throw an IOException if anyone calls setStoragePolicy" FileSystem. If the use 
doesn't throw an exception then either a) CommonFSUtils isn't passing values to 
the FileSystem it should or b) the newly added FileSystem doesn't actually 
throw and exception when called.

if the problem is a) then we haven't solved part of the problem this jira 
addresses. if the problem is b) then we also don't have confirmation that 
CommonFSUtils didn't pass the "NONE" value along to the FileSystem 
implementation.


was (Author: busbey):
{quote}
bq. CommonFSUtils should be updated to check against 
DEFER_TO_HDFS_STORAGE_POLICY

It should check against the default storage policy and let through if so. 
Currently the default policy is set to "NONE", and we give it a comprehensive 
constant name DEFER_TO_HDFS_STORAGE_POLICY
{quote}

Right now the code in CommonFSUtils just checks against the default passed into 
the call to {{setStoragePolicy}}, not against any constant. That's incorrect. 
E.g. if someone calls {{CommonFSUtils.setStoragePolicy(fs, conf, 
"get.this.storage.policy.key", "HOT")}} then if "get.this.storage.policy.key" 
isn't set it should a) use the value "HOT" and b) actually pass that value to 
the FileSystem implementation. When checking against a constant, the one it 
should check is DEFER_TO_HDFS_STORAGE_POLICY because changing our default 
policy for WALs shouldn't change which values are passed through to the 
FileSystem instance.

{quote}
bq. the test should be in TestCommonFSUtils

Agree that they should be. Checking commit history, the set-storage related 
test cases were added in TestFSUtils by HBASE-13498 and somehow left there 
during the code refactor in HBASE-18784... How about we open a follow-on JIRA 
to move all set-storage test cases into TestCommonFSUtils after this one?
{quote}

Sure. please link it here.

{quote}
bq. Shouldn't this second invocation have thrown an IOException?

Personally I think it's OK to let it fail silently with some warning log if the 
given policy is invalid or the set policy attempt failed, as the current 
implementation does. Throwing an IOE and cause region fail to open is too much 
IMHO.
{quote}

I don't mean in the region server, I mean just here in this test. the second 
call effectively uses "HOT" as the storage policy. That's a policy that we 
should give to the underlying FileSystem. The call in the test passes 
{{testFs}} as the FileSystem instance, which is an instance of the newly added 
"throw and IOException if anyone calls setStoragePolicy" FileSystem. If the use 
doesn't throw an exception then either a) CommonFSUtils isn't passing values to 
the FileSystem it should or b) the newly added FileSystem doesn't actually 
throw and exception when called.

if 

[jira] [Comment Edited] (HBASE-20691) Storage policy should allow deferring to HDFS

2018-06-08 Thread Yu Li (JIRA)


[ 
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16506121#comment-16506121
 ] 

Yu Li edited comment on HBASE-20691 at 6/8/18 3:07 PM:
---

Below is what I observed in my local run with patch v3, FYI.
{noformat}
2018-06-08 22:59:39,059 DEBUG [Time-limited test] util.TestFSUtils(350): Before 
set storage policy to NONE
2018-06-08 22:59:39,062 DEBUG [Time-limited test] util.TestFSUtils(353): After 
set storage policy to NONE
2018-06-08 22:59:39,063 WARN  [Time-limited test] util.CommonFSUtils(572): 
Unable to set storagePolicy=HOT for path=non-exist. DEBUG log level might have 
more details.
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.invokeSetStoragePolicy(CommonFSUtils.java:563)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:524)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:484)
at 
org.apache.hadoop.hbase.util.TestFSUtils.verifyNoHDFSApiInvocationForDefaultPolicy(TestFSUtils.java:356)
at 
org.apache.hadoop.hbase.util.TestFSUtils.testSetStoragePolicyDefault(TestFSUtils.java:341)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
Caused by: java.io.IOException: The setStoragePolicy method is invoked 
unexpectedly
at 
org.apache.hadoop.hbase.util.TestFSUtils$AlwaysFailSetStoragePolicyFileSystem.setStoragePolicy(TestFSUtils.java:364)
... 30 more
{noformat}


was (Author: carp84):
Below is what I observed in my local run, FYI.
{noformat}
2018-06-08 22:59:39,059 DEBUG [Time-limited test] util.TestFSUtils(350): Before 
set storage policy to NONE
2018-06-08 22:59:39,062 DEBUG [Time-limited test] util.TestFSUtils(353): After 
set storage policy to NONE
2018-06-08 22:59:39,063 WARN  [Time-limited test] util.CommonFSUtils(572): 
Unable to set storagePolicy=HOT for path=non-exist. DEBUG log level might have 
more details.
java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.invokeSetStoragePolicy(CommonFSUtils.java:563)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:524)
at 
org.apache.hadoop.hbase.util.CommonFSUtils.setStoragePolicy(CommonFSUtils.java:484)
at 
org.apache.hadoop.hbase.util.TestFSUtils.verifyNoHDFSApiInvocationForDefaultPolicy(TestFSUtils.java:356)
at 
org.apache.hadoop.hbase.util.TestFSUtils.testSetStoragePolicyDefault(TestFSUtils.java:341)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
Caused by: java.io.IOException: The setStoragePolicy method is invoked 
unexpectedly
at 
org.apache.hadoop.hbase.util.TestFSUtils$AlwaysFailSetStoragePolicyFileSystem.setStoragePolicy(TestFSUtils.java:364)
... 30 more
{noformat}

> Storage policy should allow deferring to HDFS
> -
>
> Key: HBASE-20691
> URL: https://issues.apache.org/jira/browse/HBASE-20691
> Project: HBase
>  Issue Type: Bug
>  Components: Filesystem Integration, wal
>Affects Versions: 1.5.0, 2.0.0
>Reporter: Sean Busbey
>Assignee: Yu Li
>Priority: Minor
> Fix For: 2.1.0, 1.5.0
>
> Attachments: HBASE-20691.patch, HBASE-20691.v2.patch, 
> HBASE-20691.v3.patch
>
>
> In HBase 1.1 - 1.4 we can defer storage policy decisions to HDFS by using 
> "NONE" as the storage policy in hbase configs.
> As described on this [dev@hbase thread "WAL storage policies and interactions 
> with Hadoop admin 
> tools."|https://lists.apache.org/thread.html/d220726fab4bb4c9e117ecc8f44246402dd97bfc986a57eb2237@%3Cdev.hbase.apache.org%3E]
>  we no longer have that option in 2.0.0 and 1.5.0 (as the branch is now). 
> Additionally, we can't set the policy to HOT in the event that HDFS has 
> changed the policy for a parent directory of our WALs.
> We should put back that ability. Presuming this is done by re-adopting the 
> "NONE" placeholder variable, we need to ensure that value doesn't get passed 
> to HDFS APIs. Since it isn't a valid storage policy attempting to use it will 
> cause a