[
https://issues.apache.org/jira/browse/HDFS-9129?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mingliang Liu updated HDFS-9129:
--------------------------------
Attachment: HDFS-9129.007.patch
Thank you [~wheat9] for your detailed comments. The v7 patch addresses most of
them. See response inline.
{quote}
{code}
if (status == BMSafeModeStatus.OFF) {
return;
}
{code}
1. There are multiple cases like the above. They should be asserts.
{quote}
It is safe for these methods to be called multiple times. If safe mode is not
currently on, this is a no-op. Previously we check whether the {{safeMode}} is
null before calling the respective methods. E.g.
{code:title=previous volatile and null check to allow calling multiple times}
public void checkSafeMode() {
// safeMode is volatile, and may be set to null at any time
SafeModeInfo safeMode = this.safeMode;
if (safeMode != null) {
safeMode.checkMode();
}
}
{code}
As we moved the start up safe mode to {{BlockManager}} and maintain a state
machine for safe mode status, the volatile and null {{safeMode}} trick is not
needed. Meanwhile, we must allow {{BlockManagerSafeMode}}'s methods called
multiple times without side effect.
I will explicitly elaborate this in comments for these methods.
{quote}
2. namesystem.isHaEnabled() does not change in the lifecycle of the process.
{quote}
This is a very good point. I considered this but the {{blockManager}}, which
will create the {{blockManagerSafeMode}}, is constructed before the
{{namesystem#haEnabled}} is initialized. Per offline discussion with [~wheat9]
and [~jingzhao], we can initialize the {{namesystem#haEnabled}} before
constructing the {{blockManager}}. This way, the {{namesystem.isHaEnabled}} is
not called repeatedly in the critical path.
{quote}
3. It's better to document the conditions of state transition.
{quote}
Yes, it makes a lot of sense to document the state machine transition. In v7
patch, I add a diagram in the comment.
{quote}
{code}
+ needExtension = extension > 0 &&
+ (blockThreshold > 0 || datanodeThreshold > 0);
{code}
4. This can be moved under the THRESHOLD statement and become a local variable.
{quote}
Actually it's hard, if not impossible, largely because the {{needExtension}}
should be initialized in the start status, aka {{INITALIZED}}. There is a
regression test for this case
{{TestHASafeMode#testBlocksRemovedBeforeStandbyRestart}}, brought by
[HDFS-2692].
{quote}
5. initializeReplQueuesIfNecessary() should be called only once.
{quote}
Yes, we should initialize the replication queue only once. Once called for the
first time, {{BlockManager#initializeReplQueues}} will set a flag indicating
the replication queue is initialized. We'll check this flag in
{{isPopulatingReplQueues}} before calling {{initializeReplQueues}} again. As it
is of great importance to guarantee this, I'll double check this and fix this
in next patch.
{quote}
6. safeModeStatus = SafeModeStatus.OFF should be moved to BlockManagerSafeMode.
{quote}
The {{safeModeStatus}} was for the {{FSNamesystem}} and the *OFF* here
indicates both {{FSNamesystem}} and {{BlockManager}} leave the safe mode.
{{BlockManagerSafeMode}}'s internal {{status}} was maintained in its own
{{leaveSafeMode}} method.
Per offline discussion with [~wheat9] and [~jingzhao], the better design is to
simplify the {{Namesystem}} safe mode to two flags indicating _manually_ or
_resoure low_. In this way, the safe mode check is pretty straight-forward. The
side benefit is that when we extend the current safe mode status, one more flag
will work just fine, without breaking the existing code.
{code:title=new manual and resource low safe mode flag}
private volatile boolean isInManualSafeMode = false;
private volatile boolean isInResourceLowSafeMode = false;
...
@Override
public boolean isInSafeMode() {
return isInManualSafeMode ||
isInResourceLowSafeMode ||
blockManager.isInSafeMode();
}
{code}
{quote}
7. A cleaner approach is to put the {{reached}} timestamp into the constructor
of {{SafeModeMonitor()}}.
{quote}
It's a good point to define the {{reached}} value in the monitor. The v7 patch
initializes it when the monitor starts. As the {{reached}} timestamp is partly
used out of {{SafeModeMonitor}}, e.g. in {{getSafeModeTip}} and {{checkMode}},
the easy (may not be best) way is to treat it as class field.
{quote}
8. It might be good to have additional unit tests for BlockManagerSafeMode.
{quote}
That makes perfect sense to me. I'll add new unit test named
{{TestBlockManagerSafeMode}} in the next patch.
> Move the safemode block count into BlockManager
> -----------------------------------------------
>
> Key: HDFS-9129
> URL: https://issues.apache.org/jira/browse/HDFS-9129
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Haohui Mai
> Assignee: Mingliang Liu
> Attachments: HDFS-9129.000.patch, HDFS-9129.001.patch,
> HDFS-9129.002.patch, HDFS-9129.003.patch, HDFS-9129.004.patch,
> HDFS-9129.005.patch, HDFS-9129.006.patch, HDFS-9129.007.patch
>
>
> The {{SafeMode}} needs to track whether there are enough blocks so that the
> NN can get out of the safemode. These fields can moved to the
> {{BlockManager}} class.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
