Xiaoyu Yao commented on HDFS-10906:

Thanks [~hanishakoneru] for working on this to improve the unit test coverage. 
The patch looks good to me overall. I just have a few comments.

1. Unused imports
import org.apache.hadoop.fs.FileStatus;
import org.junit.rules.Timeout;
import org.junit.runners.MethodSorters;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.FixMethodOrder;

2. We can remove the EncryptionFaultInjector if it is not used in the test. 
119       public void teardown() {
120         if (cluster != null) {
121           cluster.shutdown();
122           cluster = null;
123         }
124         EncryptionFaultInjector.instance = new EncryptionFaultInjector();
125     ...

3. verifyDelete()
We can use the Path constructor that takes two parameters to avoid concat the 
path with "/" 
203         final Path trashFile = new Path(shell.getCurrentTrashDir(path) + 
"/" +
204             path);

4. testDeleteEZWithMultipleUsers()
Some of the verification of positive cases can be simplified by verifyDelete(). 
Combining with the verifyDelete() in TestTrashWithSecureEncryptionZones, I 
think we can add it to test utilities such as DFSTestUtil  for better reuse. 

NIT: extra empty lines around 156

5. Unused imports
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

6. NIT: many instances of local variable len can be a static member of the 
{code}    final int len = 8192; {code}
7. testTrashExpunge
Can we add verification of case where expunge will delete checkpoint inside the
encryption zone when the whole encryption zone has been deleted and moved to 
directory under /user/$USER/.Trash?

> Add unit tests for Trash with HDFS encryption zones
> ---------------------------------------------------
>                 Key: HDFS-10906
>                 URL: https://issues.apache.org/jira/browse/HDFS-10906
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: encryption
>    Affects Versions: 2.8.0
>            Reporter: Xiaoyu Yao
>            Assignee: Hanisha Koneru
>         Attachments: HDFS-10906.000.patch, HDFS-10906.001.patch
> The goal is to improve unit test coverage for HDFS trash with encryption zone 
> especially under Kerberos environment. The current unit test 
> TestEncryptionZones#testEncryptionZonewithTrash() has limited coverage on 
> non-Kerberos case.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to