[
https://issues.apache.org/jira/browse/HDFS-15340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17103828#comment-17103828
]
Yiqun Lin commented on HDFS-15340:
----------------------------------
Haven't taken the deep review, but some initial review comments for the
readable of this patch:
*BalanceJob.java*
{noformat}
+ public static final Logger LOG =
+ LoggerFactory.getLogger(BalanceJob.class.getName());
{noformat}
{{BalanceJob.class.getName()}} can be simplified to {{BalanceJob.class}}
{noformat}
private BalanceJob() {}
{noformat}
I don't think this is necessary since we already define {{private
BalanceJob(Iterable<T> procedures, boolean remove)}}.
In BalanceJob#toString, can we add missed comma character in string builder
when doing the new append operation?
*BalanceProcedure.java*
{noformat}
public static final Logger LOG =
+ LoggerFactory.getLogger(BalanceProcedure.class.getName());
{noformat}
BalanceProcedure.class.getName() --> BalanceProcedure.class
It will look better if we could add some necessary comments for these variables.
{noformat}
+ private String nextProcedure;
+ private String name;
+ private long delayDuration;
+ private BalanceJob job;
{noformat}
Don't need I think and I don't find he place that will invoke following
construct method.
{noformat}
public BalanceProcedure() { }
{noformat}
*BalanceProcedureScheduler.java*
{noformat}
+ private ConcurrentHashMap<BalanceJob, BalanceJob> jobSet;
+ private LinkedBlockingQueue<BalanceJob> runningQueue;
+ private DelayQueue<DelayWrapper> delayQueue;
+ private LinkedBlockingQueue<BalanceJob> recoverQueue;
+ private Configuration conf;
+ private BalanceJournal journal;
+
+ private Thread reader;
+ private ThreadPoolExecutor workersPool;
+ private Thread rooster;
+ private Thread recoverThread;
+ private AtomicBoolean running = new AtomicBoolean(true);
{noformat}
Two suggestions for above lines:
* Can we add some necessary comments for these variables?
* Can we unified the naming pattern of thread names, like reader ->
readerThread, rooster -> roosterThread?
*HDFSJournal.java*
{noformat}
+/**
+ * Journal based on HDFS.
+ */
+public class HDFSJournal implements BalanceJournal {
{noformat}
The class HDFSJournal looks a little confused, can we rename this to a more
readable name like BalanceJournalInfo?
In addition, please add more description for this java doc description.
Can we print some tracking log in saveJob/recoverJob/listAllJobs/clear methods?
It will let users known detailed operation of Balancer job journal operations.
*MultiPhaseProcedure.java*
* LOG.info("phase {}", currentPhase); --> LOG.info("Current phase {}",
currentPhase);
* Not needed: {{public MultiPhaseProcedure() {}}}
*RecordProcedure.java*
Not needed: {{public RecordProcedure() {}}}
*RetryProcedure.java*
Not needed: {{public RetryProcedure() {}}}
I will give my further detailed review comments soon.
> RBF: Balance data across federation namespaces with DistCp and snapshot diff
> / Step 1: The State Machine(BalanceProcedureScheduler)
> -----------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDFS-15340
> URL: https://issues.apache.org/jira/browse/HDFS-15340
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Jinglun
> Assignee: Jinglun
> Priority: Major
> Attachments: HDFS-15340.001.patch, HDFS-15340.002.patch
>
>
> Patch in HDFS-15294 is too big to review so we split it into 2 patches. This
> is the first one. Detail can be found at HDFS-15294.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
