Activity

I'm working on a patch to wrap the CopyReadException only once. However, I found it's hard to write a unit test which will fail. The current unit tests are not able to detect this bug because the map method throws an CopyReadException when calling getFileStatus, before it calls the copyFileWithRetry. This makes sense as the file is deleted before the copy operation. This way the unit tests pass with the following condition true (in case of ignoreFailures):

Mingliang Liu
added a comment - 09/Oct/15 22:06 Hi Gera Shegalov , thanks for reporting this.
I'm working on a patch to wrap the CopyReadException only once. However, I found it's hard to write a unit test which will fail. The current unit tests are not able to detect this bug because the map method throws an CopyReadException when calling getFileStatus , before it calls the copyFileWithRetry . This makes sense as the file is deleted before the copy operation. This way the unit tests pass with the following condition true (in case of ignoreFailures):
if (ignoreFailures && exception.getCause() instanceof
RetriableFileCopyCommand.CopyReadException
Should we address the orthogonal ignoreFailures mutually exclusive with the atomic option separately?

Haohui Mai
added a comment - 10/Oct/15 10:24 Reopening the jira.
Sorry for the noise – I realized that I haven't posted my +1 on the patch before committing. Taking a closer look the patch, it does not seem to really address the problem. I'm going to revert it now.

Hi Mingliang Liu, thanks for working on the patch. Regarding the unit test you should consider a full distcp test where you create a file and make it unusable by setting 000 permission. DistCp should be run doAs non-cluster-root user.

Should we address the orthogonal ignoreFailures mutually exclusive with the atomic option separately?

Gera Shegalov
added a comment - 11/Oct/15 22:35 Hi Mingliang Liu , thanks for working on the patch. Regarding the unit test you should consider a full distcp test where you create a file and make it unusable by setting 000 permission. DistCp should be run doAs non-cluster-root user.
Should we address the orthogonal ignoreFailures mutually exclusive with the atomic option separately?
Sure, will you file it?

Mingliang Liu
added a comment - 12/Oct/15 17:40 Thanks Haohui Mai for reviewing this patch. I should make it clear that it was not to address the whole issue when I upload the patch. Sorry for the confusion.
As Gera Shegalov suggested, I'll update the patch with tests enabled. I filed a new issue HADOOP-12473 about the atomic option for ignoring failures.

As Gera Shegalov suggested, the v1 patch changes unit test helper method doTestIgnoreFailures by creating a file and making it unusable by setting 000 permission. The unit test testIgnoreFailures can not pass in trunk code without this patch, as it should wrap the CopyReadException only once.

Mingliang Liu
added a comment - 31/Oct/15 22:05 As Gera Shegalov suggested, the v1 patch changes unit test helper method doTestIgnoreFailures by creating a file and making it unusable by setting 000 permission. The unit test testIgnoreFailures can not pass in trunk code without this patch, as it should wrap the CopyReadException only once.

Thanks Gera Shegalov for your review and comments. The v4 patch moves the doTestIgnoreFailures() method back to its old position.

The patch mainly changes helper method doTestIgnoreFailures by creating a file and making it unusable by setting 000 permission. The unit test testIgnoreFailures can not pass in trunk code without this patch, as it should wrap the CopyReadException only once.

Mingliang Liu
added a comment - 27/Jan/16 22:15 Thanks Gera Shegalov for your review and comments. The v4 patch moves the doTestIgnoreFailures() method back to its old position.
The patch mainly changes helper method doTestIgnoreFailures by creating a file and making it unusable by setting 000 permission. The unit test testIgnoreFailures can not pass in trunk code without this patch, as it should wrap the CopyReadException only once.

Per-offline discussion with Jing Zhao, the v5 patch relaxes the need of wrapping a CopyReadException exception, which happens in multiple nested call paths, and checks if any Throwable in the exception chain matches CopyReadException exception type.

Jing Zhao
added a comment - 03/May/16 23:18 Instead of replacing the old unit test (which deletes some source files to generate failures), maybe we can add a new unit test. +1 after addressing the comment.