HIVE-19248: REPL LOAD couldn't copy file from source CM path and also doesn't throw error if file copy fails. #342
Conversation
| if (scheme == null) { | ||
| // no scheme - use default file system uri | ||
| scheme = fsUri.getScheme(); | ||
| authority = fsUri.getAuthority(); |
There was a problem hiding this comment.
Path makeQualified(URI defaultUri, Path workingDir) .does not use the same logic ..is it intentional ?
There was a problem hiding this comment.
What is different in this logic?
| if(bytes == null) { | ||
| throw new IllegalArgumentException("bytes == null"); | ||
| } else { | ||
| StringBuilder s = new StringBuilder(); |
There was a problem hiding this comment.
why hadoop's StringUtils.byteToHexString is not used ?
There was a problem hiding this comment.
Conflicting StringUtils classes if I import hadoop's class. So, just moved the implementation here as it is standalone metastore and even other methods does the same.
| + "Source File: " + srcFile.getSourcePath()); | ||
| throw new IOException("File copy failed and likely source file is deleted or modified."); | ||
| } | ||
| pathList.add(srcFile.getEffectivePath()); |
There was a problem hiding this comment.
srcPath won't match with srcFile.getEffectivePath() as we changed the isUseSrcFile flag.
| } | ||
|
|
||
| // If files copy fail after several attempts, then throw error | ||
| if (!pathList.isEmpty() || isCopyError) { |
There was a problem hiding this comment.
in what scenario path list will be empty with copy error set to true ?
There was a problem hiding this comment.
not possible. I'm checking for path list not empty or copy error, then retry.
| int repeat = 0; | ||
| List<Path> pathList = Lists.transform(fileList, ReplChangeManager.FileInfo::getEffectivePath); | ||
| boolean isCopyError = false; | ||
| List<Path> pathList = Lists.transform(srcFileList, ReplChangeManager.FileInfo::getEffectivePath); |
There was a problem hiding this comment.
if we do transform here ..is it possible that some of the entries will be null ?
may be transform is not required here ?
There was a problem hiding this comment.
nope... here it won't return null. but just traverse through the list and get the effective path. So, cannot skip the transform.
| if (!retryFileInfoList.isEmpty()) { | ||
| doCopyRetry(sourceFs, retryFileInfoList, destinationFs, destination, useRegularCopy); | ||
| } | ||
| // Copy files with retry logic on failure |
There was a problem hiding this comment.
retry logic on failure and source file modification
There was a problem hiding this comment.
Will fix this if any code change needed for this patch. :)
| result = ReplChangeManager.encodeFileUri(files.get(i), chksums != null ? chksums.get(i) : null, null); | ||
| } catch (IOException e) { | ||
| // If thrown IOException, it means, CM root is not set. So, just return original path. | ||
| result = files.get(i); |
There was a problem hiding this comment.
i think .. ReplChangeManager should handle (null check is already there) ..cm_root not set scenario ..here in this layer we should throw exception for any failure
There was a problem hiding this comment.
This method is inherited and cannot change the definition. So, exception is caught and handle it. Yes, it is dead code for now.
… doesn't throw error if file copy fails.
2 participants
No description provided.