[
https://issues.apache.org/jira/browse/HDFS-11336?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15880001#comment-15880001
]
Uma Maheswara Rao G commented on HDFS-11336:
--------------------------------------------
Hi [~yuanbo] , thank you for updating the patch. Overall patch looks good, I
have the following comments though
# .
{code}
try {
+ namesystem.getFSDirectory().removeSatisfyXattr(id);
+ } catch (Exception ex) {
+ LOG.warn("Failed to remove satisfy "
+ + "xattr for collection id " + id, ex);
+ }
{code}
Is there a specific reason you handled Exception instead of IOException ?
# .
{code}
removeSatisfyXattr
{code}
would it be good to make name more clear? may be like
removeStoragePolicySatisfierXattr or removeSPSXattr ? also javadoc can mention
clearly that.
# .
{code}
+ * 3. satisfy storage policy of file1 again
+ * 4. make sure step 3 works as expected.
{code}
Since you tried to add comments clearly, it may be worth mentioning what you
are expecting.
# .
typo: satisfy attr —> ….. Xattr
# .
{code}
* @return
+ * @throws IOException
{code}
return parameter missed
# .
{code}
- } catch (InterruptedException ie) {
+ } catch (Exception e) {
LOG.info("BlocksStorageMovementAttemptResultMonitor thread "
- + "is interrupted.", ie);
+ + "received exception and exiting.", e);
{code}
generally getting interrupted exception is positive case so that log can be
info. But if you get some other exception then it can’t be info. It should be
warn right.
So, how about having check like if exception is interrupted, then log info else
log warn?
# .
{code}
// Remove xattr for the track id.
+ namesystem.getFSDirectory().removeSatisfyXattr(
+ storageMovementAttemptedResult.getTrackId());
{code}
One design level suggestion is, instead of accessing name system internals all
around, how about block manager talk to name system?
I mean, let BlockStorageMovementAttemptedItems hold block manager instead of
name system. then when you want to call remove Xattr, then call
blkManager.remove*Xattr. Now blkManger can delegate to name system to do actual
functioning.
> [SPS]: Remove xAttrs when movements done or SPS disabled
> --------------------------------------------------------
>
> Key: HDFS-11336
> URL: https://issues.apache.org/jira/browse/HDFS-11336
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, namenode
> Reporter: Yuanbo Liu
> Assignee: Yuanbo Liu
> Attachments: HDFS-11336-HDFS-10285.001.patch,
> HDFS-11336-HDFS-10285.002.patch, HDFS-11336-HDFS-10285.003.patch
>
>
> 1. When we finish the movement successfully, we should clean Xattrs.
> 2. When we disable SPS dynamically, we should clean Xattrs
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
