[
https://issues.apache.org/jira/browse/HDFS-6808?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14139386#comment-14139386
]
Colin Patrick McCabe commented on HDFS-6808:
--------------------------------------------
ReconfigurableBase: you have a bunch of comments in here that would be better
as JavaDoc.
For example:
{code}
// The timestamp when the <code>reconfigThread</code> starts.
private long startTime = 0;
// The timestamp when the <code>reconfigThread</code> finishes.
private long endTime = 0;
{code}
instead you could have:
{code}
/**
* The timestamp when the <code>reconfigThread</code> starts.
*/
private long startTime = 0;
/**
* The timestamp when the <code>reconfigThread</code> finishes.
*/
private long endTime = 0;
{code}
{{ClientDatanodeProtocol}}: this looks good overall. I wonder if
"StartReconfigurationRequestProto", etc. would be better than
"StartReconfigureRequestProto"?
{code}
message StartReconfigureRequestProto {
}
message StartReconfigureResponseProto {
enum StartReconfigureResult {
SUCCESS = 0;
SERVER_STOPPED = 1;
EXISTED = 2;
}
required StartReconfigureResult result = 1;
}
{code}
I guess this is a matter of style, but I don't think you need this enum. Just
throw an exception (handled specially by our RPC system) when the server is
stopped or when there is already an ongoing reconfiguration. If you throw an
IOException, it will make it through to the other side.
{code}
/**
* Start a reconfiguration task to reload configuration in background.
*/
public StartReconfigureResult startReconfigureTask() {
synchronized (this) {
if (!shouldRun) {
LOG.warn("The server is stopping.");
return StartReconfigureResult.SERVER_STOPPED;
}
if (reconfigThread != null) {
LOG.warn("Another reconfigure task is running.");
return StartReconfigureResult.EXISTED;
}
reconfigThread = new ReconfigureThread(this);
reconfigThread.start();
startTime = Time.monotonicNow();
}
return StartReconfigureResult.SUCCESS;
}
{code}
Similar to the protobuf code, this could simply throw IOException rather than
using an enum. Also, since it's pretty much all synchronized (except the
return statement?) it could just be a synchronized method.
In {{ClientDatanodeProtocol.java}}:
{code}
/**
* Asynchronously reload configuration on disk and apply changes.
*/
StartReconfigureResult startReconfigure() throws IOException;
{code}
Similar to the above, this could just return void. (If there is already a
reconfiguration in progress, we can throw an IOE.)
{code}
message GetReconfigureStatusResultProto {
required string name = 1;
required string oldValue = 2;
required string newValue = 3;
optional string errorMessage = 4; // It is empty if success.
}
message GetReconfigureStatusResponseProto {
required int64 startTime = 1;
optional int64 endTime = 2;
repeated GetReconfigureStatusResultProto status = 3;
}
{code}
{{GetReconfigureStatusResultProto}} is kind of a confusing name. This isn't
really a "result," it's a configuration key that we're changing. How about
calling it {{GetReconfigurationStatusConfigChangeProto}}? Also, it seems like
{{newValue}} should be marked {{optional}} to fit in with the idea that a
configuration key could be removed (that's why you use {{Optional}} in other
places, right?)
In {{ReconfigurableBase#ReconfigureTaskStatus}}, we have:
{code}
public final Map<PropertyChange, Optional<String>> getStatus() {
return status;
}
{code}
This is a little concerning since this map is mutable... the caller could
theoretically modify this, causing chaos. Can we wrap this in an
{{ImmutableCollection}} or {{ImmutableMap}} to prevent this?
> Add command line option to ask DataNode reload configuration.
> -------------------------------------------------------------
>
> Key: HDFS-6808
> URL: https://issues.apache.org/jira/browse/HDFS-6808
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode
> Affects Versions: 2.5.0
> Reporter: Lei (Eddy) Xu
> Assignee: Lei (Eddy) Xu
> Attachments: HDFS-6808.000.combo.patch, HDFS-6808.000.patch,
> HDFS-6808.001.combo.patch, HDFS-6808.001.patch, HDFS-6808.002.combo.patch,
> HDFS-6808.002.patch, HDFS-6808.003.combo.txt, HDFS-6808.003.patch,
> HDFS-6808.004.combo.patch, HDFS-6808.004.patch, HDFS-6808.005.combo.patch,
> HDFS-6808.005.patch, HDFS-6808.006.combo.patch, HDFS-6808.006.patch,
> HDFS-6808.007.combo.patch, HDFS-6808.007.patch
>
>
> The workflow of dynamically changing data volumes on DataNode is
> # Users manually changed {{dfs.datanode.data.dir}} in the configuration file
> # User use command line to notify DN to reload configuration and updates its
> volumes.
> This work adds command line support to notify DN to reload configuration.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
