[
https://issues.apache.org/jira/browse/HBASE-20691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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 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.
> 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/d220726fab4bb4c9e117ecc8f44246402dd97bfc986a57eb22311117@%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)
