HDDS-5149 when source datanode download container tar from target datanode,but the target datanode container file missing,import error #2196
Conversation
…anode,but the target datanode container file missing,import error
…anode,but the target datanode container file missing,import error
…anode,but the target datanode container file missing,import error
…anode,but the target datanode container file missing,import error
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @cchenax for working on this. The idea that container export/import may perform more sanity checks sounds good to me.
| if (!containerData.getDbFile().exists()) { | ||
| LOG.warn("DBfile {} not exist", | ||
| containerData.getDbFile().toPath().toString()); | ||
| return; | ||
| } else if (!new File(containerData.getChunksPath()).exists()) { | ||
| LOG.warn("Chunkfile {} not exist", | ||
| containerData.getDbFile().toPath().toString()); | ||
| return; | ||
| } else if (!container.getContainerFile().exists()) { | ||
| LOG.warn("Containerfile {} not exist", | ||
| containerData.getDbFile().toPath().toString()); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it make sense to utilize Container.scanMetadata() instead? More complete checks without introducing duplication.
There was a problem hiding this comment.
Ok,I will change it.thank you very much!
| container.importContainerData(fis, packer); | ||
| } catch (Exception ex) { | ||
| assertTrue(ex instanceof NullPointerException); |
There was a problem hiding this comment.
- This passes even if
importContainerDatadoes not throw, which is not expected. - Some exception other than
NullPointerExceptionmay better describe the error. Assert.assertThrowsorLambdaTestUtils.interceptcould provide more details if the assertion fails (eg. if not the expected kind of exception was thrown).
| // if tar is null, the tar size is 45 bytes | ||
| if (bytes <= 45) { | ||
| task.setStatus(Status.FAILED); | ||
| LOG.warn("Container {} is downloaded with size {}, " + | ||
| "if size less than 45 bytes the tar file is null", | ||
| containerID, bytes); |
There was a problem hiding this comment.
I think these kind of tar-specific details should be handled by TarContainerPacker.
There was a problem hiding this comment.
hello,I think it should be check in the DownloadAndImportReplicator,because of the source datanode will through outputstream to the tar,if tar is null,the task status should be set failed,the tar size check in TarContainerPacker I think is not useful.
There was a problem hiding this comment.
Please consider the case if someone wanted to introduce a new implementation of ContainerPacker, eg. based on zip. This check would not make sense, or at least would need different number of minimum bytes, for the other implementation.
|
Continued in #2376. |
2 participants
What changes were proposed in this pull request?
fix if the metadatafile or chunksfile missing import export container error
What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/HDDS/issues/HDDS-5149?filter=addedrecently
How was this patch tested?
unittest