[
https://issues.apache.org/jira/browse/HADOOP-18012?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17702484#comment-17702484
]
ASF GitHub Bot commented on HADOOP-18012:
-----------------------------------------
saxenapranav commented on code in PR #5488:
URL: https://github.com/apache/hadoop/pull/5488#discussion_r1141679607
##########
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);
+ Mockito.doReturn(404).when(mockHttp404Op).getStatusCode();
+
Mockito.doNothing().when(mockHttp404Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp404Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp404Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp404Op).getMethod();
+ Mockito.doReturn("Source Path not
found").when(mockHttp404Op).getStorageErrorMessage();
+
Mockito.doReturn("SourcePathNotFound").when(mockHttp404Op).getStorageErrorCode();
+
+
+ // // mock object representing the 500 timeout result for first try of
rename
+ AbfsHttpOperation mockHttp500Op = Mockito.mock(AbfsHttpOperation.class);
+ Mockito.doReturn(500).when(mockHttp500Op).getStatusCode();
+ Mockito.doThrow(IOException.class)
+ .when(mockHttp500Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp500Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp500Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp500Op).getMethod();
+
Mockito.doReturn("ClientTimeoutError").when(mockHttp500Op).getStorageErrorCode();
Review Comment:
lets not have two mocks. As its something which needs to verify server
orchestration, lets not just have it as a unit test. Lets have it an
integration test :).
##########
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);
+ Mockito.doReturn(404).when(mockHttp404Op).getStatusCode();
+
Mockito.doNothing().when(mockHttp404Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp404Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp404Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp404Op).getMethod();
+ Mockito.doReturn("Source Path not
found").when(mockHttp404Op).getStorageErrorMessage();
+
Mockito.doReturn("SourcePathNotFound").when(mockHttp404Op).getStorageErrorCode();
+
+
+ // // mock object representing the 500 timeout result for first try of
rename
+ AbfsHttpOperation mockHttp500Op = Mockito.mock(AbfsHttpOperation.class);
+ Mockito.doReturn(500).when(mockHttp500Op).getStatusCode();
+ Mockito.doThrow(IOException.class)
+ .when(mockHttp500Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp500Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp500Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp500Op).getMethod();
+
Mockito.doReturn("ClientTimeoutError").when(mockHttp500Op).getStorageErrorCode();
Review Comment:
Please refer following code for actually using server in picture:
```
Mockito.doAnswer(answer -> {
AbfsRestOperation op = new
AbfsRestOperation(AbfsRestOperationType.RenamePath,
spyClient, HTTP_METHOD_PUT, answer.getArgument(0),
answer.getArgument(1));
AbfsRestOperation spiedOp = Mockito.spy(op);
addSpyBehavior(spiedOp, op, spyClient);
return spiedOp;
}).when(spyClient).createRenameRestOperation(nullable(URL.class),
nullable(List.class));
private void addSpyBehavior(final AbfsRestOperation spiedOp, final
AbfsRestOperation normalOp, AbfsClient client)
throws IOException {
AbfsHttpOperation abfsHttpOperation =
Mockito.spy(normalOp.createHttpOperation());
AbfsHttpOperation normalOp1 = normalOp.createHttpOperation();
normalOp1.getConnection().setRequestProperty(HttpHeaderConfigurations.AUTHORIZATION,
client.getAccessToken());
AbfsHttpOperation normalOp2 = normalOp.createHttpOperation();
normalOp2.getConnection().setRequestProperty(HttpHeaderConfigurations.AUTHORIZATION,
client.getAccessToken());
int[] hits = new int[1];
hits[0] = 0;
Mockito.doAnswer(answer -> {
if(hits[0] == 0) {
mockIdempotencyIssueBehaviours(abfsHttpOperation, normalOp1);
hits[0]++;
return abfsHttpOperation;
}
hits[0]++;
return normalOp2;
}).when(spiedOp).createHttpOperation();
}
private void mockIdempotencyIssueBehaviours(final AbfsHttpOperation
abfsHttpOperation,
final AbfsHttpOperation normalOp)
throws IOException {
Mockito.doAnswer(answer -> {
normalOp.sendRequest(answer.getArgument(0), answer.getArgument(1),
answer.getArgument(2));
normalOp.processResponse(answer.getArgument(0), answer.getArgument(1),
answer.getArgument(2));
throw new SocketException("connection-reset");
}).when(abfsHttpOperation).sendRequest(Mockito.nullable(byte[].class),
Mockito.nullable(int.class), Mockito.nullable(int.class));
}
```
##########
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);
+ Mockito.doReturn(404).when(mockHttp404Op).getStatusCode();
+
Mockito.doNothing().when(mockHttp404Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp404Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp404Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp404Op).getMethod();
+ Mockito.doReturn("Source Path not
found").when(mockHttp404Op).getStorageErrorMessage();
+
Mockito.doReturn("SourcePathNotFound").when(mockHttp404Op).getStorageErrorCode();
+
+
+ // // mock object representing the 500 timeout result for first try of
rename
+ AbfsHttpOperation mockHttp500Op = Mockito.mock(AbfsHttpOperation.class);
+ Mockito.doReturn(500).when(mockHttp500Op).getStatusCode();
+ Mockito.doThrow(IOException.class)
+ .when(mockHttp500Op).processResponse(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+
Mockito.doNothing().when(mockHttp500Op).setRequestProperty(nullable(String.class),
nullable(String.class));
+
Mockito.doNothing().when(mockHttp500Op).sendRequest(nullable(byte[].class),
Mockito.any(int.class), Mockito.any(int.class));
+ Mockito.doReturn("PUT").when(mockHttp500Op).getMethod();
+
Mockito.doReturn("ClientTimeoutError").when(mockHttp500Op).getStorageErrorCode();
Review Comment:
refer to commit
https://github.com/saxenapranav/hadoop/commit/5247e12bf2b96f8397d4c6df0865117a18dd13fd
> ABFS: Enable config controlled ETag check for Rename idempotency
> ----------------------------------------------------------------
>
> Key: HADOOP-18012
> URL: https://issues.apache.org/jira/browse/HADOOP-18012
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.3.2
> Reporter: Sneha Vijayarajan
> Assignee: Sree Bhattacharyya
> Priority: Major
> Labels: pull-request-available
>
> ABFS driver has a handling for rename idempotency which relies on LMT of the
> destination file to conclude if the rename was successful or not when source
> file is absent and if the rename request had entered retry loop.
> This handling is incorrect as LMT of the destination does not change on
> rename.
> This Jira will track the change to undo the current implementation and add a
> new one where for an incoming rename operation, source file eTag is fetched
> first and then rename is done only if eTag matches for the source file.
> As this is going to be a costly operation given an extra HEAD request is
> added to each rename, this implementation will be guarded over a config and
> can enabled by customers who have workloads that do multiple renames.
> Long term plan to handle rename idempotency without HEAD request is being
> discussed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
