saxenapranav commented on code in PR #5494:
URL: https://github.com/apache/hadoop/pull/5494#discussion_r1141630233
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -105,6 +106,11 @@ public class AbfsClient implements Closeable {
private final ListeningScheduledExecutorService executorService;
+ /**
+ * Enable resilient rename.
+ */
+ private boolean renameResilience;
Review Comment:
nit: lets have it final.
##########
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 {}",
Review Comment:
this can come even if op was retried and statusCode is not equal to 404
##########
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)) {
Review Comment:
As per code in renamePath(),
```
isDir = !isNotEmpty(sourceEtag)
```
Should we use the same relation in this method to reduce confusion, instead
of having a new argument `isDir`.
##########
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;
+ }
Review Comment:
this becomes equavalent to:
```
op.isARetriedRequest() || op.getResult().getStatusCode !=
HttpURLConnection.HTTP_NOT_FOUND
```
The older condition:
```
if ((op.isARetriedRequest())
&& (op.getResult().getStatusCode() ==
HttpURLConnection.HTTP_NOT_FOUND)
```
is changed.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsRenameRetryRecovery.java:
##########
@@ -123,6 +138,112 @@ public void testRenameFailuresDueToIncompleteMetadata()
throws Exception {
}
+ AbfsClient getMockAbfsClient() throws IOException {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ // specifying AbfsHttpOperation mock behavior
+
+ // mock object representing the 404 path not found result
+ AbfsHttpOperation mockHttp404Op = Mockito.mock(AbfsHttpOperation.class);
Review Comment:
It would be awesome if we can use server integration in picture. Have added
a comment on @sreeb-msft 's pr
https://github.com/apache/hadoop/pull/5488#discussion_r1141679607. Suggested
changes in
https://github.com/saxenapranav/hadoop/commit/5247e12bf2b96f8397d4c6df0865117a18dd13fd
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java:
##########
@@ -46,6 +46,9 @@
import javax.annotation.Nullable;
import org.apache.hadoop.classification.VisibleForTesting;
+import org.apache.hadoop.fs.EtagSource;
+import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation;
+import org.apache.hadoop.fs.azurebfs.services.AbfsRestOperation;
Review Comment:
imports not required.
--
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]
