[jira] [Commented] (HDFS-10313) Distcp does not check the order of snapshot names passed to -diff

[Yongjun Zhang (JIRA)]

    [ 
https://issues.apache.org/jira/browse/HDFS-10313?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15250438#comment-15250438
 ] 

Yongjun Zhang commented on HDFS-10313:
--------------------------------------

Hi [~linyiqun],

Thanks a lot for working on this issue! 

I looked into your patch, it looks pretty good. I have a few comments, largely 
cosmetic things:

1. It may be better to say "Snapshot <to> should be newer than <from>" in the 
following exception. Replace <from> <to> with the real names.

 throw new HadoopIllegalArgumentException(
            "The toSnapshot file should be newer than fromSnapshot file");

2.
{code}
   } catch (FileNotFoundException nfe) {
      DistCp.LOG.warn("The snapshot file not be found.", nfe);
    }

{code}
We should return false here. Or maybe we simply throw 
{{InvalidInputException}}, with nfe as the cause. I actually think the latter 
is better.

3. In {{createAndSubmitJob()}} method,
{code}
       if (distCpSync.sync()) {
          createInputFileListingWithDiff(job, distCpSync);
        } else {
          inputOptions.disableUsingDiff();
        }
{code}
I'd suggest that in the else block, we don't disable using diff, and simply 
issue error with clear message, and throw {{InvalidInputException}}  to be 
caught at {{DistCp#run}} method, thus quitting DistCp.

Hi [~jingzhao], the old behavior is that we fallback to regular distcp if sync 
failed, my suggested change would change the old behavior so no fallback would 
happen, I believe it's safer, do you agree?

4.
{code}
public void testSyncOfTimeChecking() throws Exception {
{code}
Suggest to change the test name to {{testSyncSnapshotTimeStampChecking()}}

Thanks.


> Distcp does not check the order of snapshot names passed to -diff
> -----------------------------------------------------------------
>
>                 Key: HDFS-10313
>                 URL: https://issues.apache.org/jira/browse/HDFS-10313
>             Project: Hadoop HDFS
>          Issue Type: Bug
>          Components: distcp
>            Reporter: Yongjun Zhang
>            Assignee: Lin Yiqun
>         Attachments: HDFS-10313.001.patch
>
>
> This jira is to propose adding a check to distcp, when {{-diff s1 s2}} is 
> passed, we need to ensure that s2 is newer than s1, otherwise, abort with a 
> informative error message.
> This is the result of my offline discussion with [~jingzhao] on HDFS-9820. 
> Thanks Jing.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)