mukund-thakur commented on code in PR #5494:
URL: https://github.com/apache/hadoop/pull/5494#discussion_r1141673602
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java:
##########
@@ -76,6 +76,8 @@ public FilterFileSystem(FileSystem fs) {
this.statistics = fs.statistics;
}
+
Review Comment:
cut extra lines?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() {
* Exceptions raised in the probe of the destination are swallowed,
* so that they do not interfere with the original rename failures.
* @param source source path
+ * @param sourceEtag etag of source file. may be null or empty
* @param op Rename request REST operation response with non-null HTTP
response
* @param destination rename destination path
- * @param sourceEtag etag of source file. may be null or empty
* @param tracingContext Tracks identifiers for request header
+ * @param isDir is the source a file or directory
* @return true if the file was successfully copied
*/
public boolean renameIdempotencyCheckOp(
final String source,
final String sourceEtag,
final AbfsRestOperation op,
final String destination,
- TracingContext tracingContext) {
+ TracingContext tracingContext,
+ final boolean isDir) {
Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP
response");
- if ((op.isARetriedRequest())
- && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND)
- && isNotEmpty(sourceEtag)) {
-
+ if (!(op.isARetriedRequest())
+ && (op.getResult().getStatusCode() ==
HttpURLConnection.HTTP_NOT_FOUND)) {
+ // not an error
+ return false;
+ }
+ LOG.debug("Source not found on retry of rename({}, {}) isDir {} etag {}",
+ source, destination, isDir, sourceEtag);
+ if (isDir) {
+ // directory recovery is not supported.
+ // log and fail.
+ LOG.info("rename directory {} to {} failed; unable to recover",
+ source, destination);
+ return false;
+ }
+ if (isNotEmpty(sourceEtag)) {
// Server has returned HTTP 404, which means rename source no longer
// exists. Check on destination status and if its etag matches
// that of the source, consider it to be a success.
Review Comment:
these comments not valid here.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() {
* Exceptions raised in the probe of the destination are swallowed,
* so that they do not interfere with the original rename failures.
* @param source source path
+ * @param sourceEtag etag of source file. may be null or empty
* @param op Rename request REST operation response with non-null HTTP
response
* @param destination rename destination path
- * @param sourceEtag etag of source file. may be null or empty
* @param tracingContext Tracks identifiers for request header
+ * @param isDir is the source a file or directory
* @return true if the file was successfully copied
*/
public boolean renameIdempotencyCheckOp(
final String source,
final String sourceEtag,
final AbfsRestOperation op,
final String destination,
- TracingContext tracingContext) {
+ TracingContext tracingContext,
+ final boolean isDir) {
Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP
response");
- if ((op.isARetriedRequest())
- && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND)
- && isNotEmpty(sourceEtag)) {
-
+ if (!(op.isARetriedRequest())
Review Comment:
add a comment here.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -613,39 +644,58 @@ private void incrementAbfsRenamePath() {
* Exceptions raised in the probe of the destination are swallowed,
* so that they do not interfere with the original rename failures.
* @param source source path
+ * @param sourceEtag etag of source file. may be null or empty
* @param op Rename request REST operation response with non-null HTTP
response
* @param destination rename destination path
- * @param sourceEtag etag of source file. may be null or empty
* @param tracingContext Tracks identifiers for request header
+ * @param isDir is the source a file or directory
* @return true if the file was successfully copied
*/
public boolean renameIdempotencyCheckOp(
final String source,
final String sourceEtag,
final AbfsRestOperation op,
final String destination,
- TracingContext tracingContext) {
+ TracingContext tracingContext,
+ final boolean isDir) {
Preconditions.checkArgument(op.hasResult(), "Operations has null HTTP
response");
- if ((op.isARetriedRequest())
- && (op.getResult().getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND)
- && isNotEmpty(sourceEtag)) {
-
+ if (!(op.isARetriedRequest())
+ && (op.getResult().getStatusCode() ==
HttpURLConnection.HTTP_NOT_FOUND)) {
+ // not an error
+ return false;
+ }
+ LOG.debug("Source not found on retry of rename({}, {}) isDir {} etag {}",
+ source, destination, isDir, sourceEtag);
+ if (isDir) {
+ // directory recovery is not supported.
+ // log and fail.
+ LOG.info("rename directory {} to {} failed; unable to recover",
+ source, destination);
+ return false;
+ }
+ if (isNotEmpty(sourceEtag)) {
// Server has returned HTTP 404, which means rename source no longer
// exists. Check on destination status and if its etag matches
// that of the source, consider it to be a success.
- LOG.debug("rename {} to {} failed, checking etag of destination",
+ // the source tag was either passed in from a manifest commit or
+ // retrieved when rename recovery is enabled.
+ LOG.info("rename {} to {} failed, checking etag of destination",
source, destination);
try {
final AbfsRestOperation destStatusOp = getPathStatus(destination,
false, tracingContext);
final AbfsHttpOperation result = destStatusOp.getResult();
- return result.getStatusCode() == HttpURLConnection.HTTP_OK
+ final boolean recovered = result.getStatusCode() ==
HttpURLConnection.HTTP_OK
&& sourceEtag.equals(extractEtagHeader(result));
- } catch (AzureBlobFileSystemException ignored) {
+ LOG.info("File rename has taken place: recovery completed");
Review Comment:
this log statement is incorrect. It will depend on the value of the
recovered flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
