Skip to content

Commit da81cc8

Browse files
bhattmanish98bhattmanish98
committed
Changes as per comments given
1 parent 7c71345 commit da81cc8

File tree

4 files changed

+72
-50
lines changed

4 files changed

+72
-50
lines changed

‎hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
8484
import static java.net.HttpURLConnection.HTTP_OK;
8585
import static java.net.HttpURLConnection.HTTP_PRECON_FAILED;
86+
import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS;
8687
import static org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore.extractEtagHeader;
8788
import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.ACQUIRE_LEASE_ACTION;
8889
import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.AND_MARK;
@@ -441,8 +442,12 @@ public void createNonRecursivePreCheck(Path parentPath,
441442
if (isAtomicRenameKey(parentPath.toUri().getPath())) {
442443
takeGetPathStatusAtomicRenameKeyAction(parentPath, tracingContext);
443444
}
444-
getPathStatus(parentPath.toUri().getPath(), false,
445-
tracingContext, null);
445+
try {
446+
getPathStatus(parentPath.toUri().getPath(), false,
447+
tracingContext, null);
448+
} finally {
449+
getAbfsCounters().incrementCounter(CALL_GET_FILE_STATUS, 1);
450+
}
446451
} catch (AbfsRestOperationException ex) {
447452
if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
448453
throw new FileNotFoundException("Cannot create file "
@@ -804,22 +809,26 @@ public AbfsClientRenameResult renamePath(final String source,
804809
BlobRenameHandler blobRenameHandler = getBlobRenameHandler(source,
805810
destination, sourceEtag, isAtomicRenameKey(source), tracingContext
806811
);
807-
if (blobRenameHandler.execute()) {
808-
final AbfsUriQueryBuilder abfsUriQueryBuilder
809-
= createDefaultUriQueryBuilder();
810-
final URL url = createRequestUrl(destination,
811-
abfsUriQueryBuilder.toString());
812-
final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
813-
final AbfsRestOperation successOp = getSuccessOp(
814-
AbfsRestOperationType.RenamePath, HTTP_METHOD_PUT,
815-
url, requestHeaders);
816-
return new AbfsClientRenameResult(successOp, true, false);
817-
} else {
818-
throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR,
819-
AzureServiceErrorCode.UNKNOWN.getErrorCode(),
820-
ERR_RENAME_BLOB + source + SINGLE_WHITE_SPACE + AND_MARK
821-
+ SINGLE_WHITE_SPACE + destination,
822-
null);
812+
try {
813+
if (blobRenameHandler.execute()) {
814+
final AbfsUriQueryBuilder abfsUriQueryBuilder
815+
= createDefaultUriQueryBuilder();
816+
final URL url = createRequestUrl(destination,
817+
abfsUriQueryBuilder.toString());
818+
final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
819+
final AbfsRestOperation successOp = getSuccessOp(
820+
AbfsRestOperationType.RenamePath, HTTP_METHOD_PUT,
821+
url, requestHeaders);
822+
return new AbfsClientRenameResult(successOp, true, false);
823+
} else {
824+
throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR,
825+
AzureServiceErrorCode.UNKNOWN.getErrorCode(),
826+
ERR_RENAME_BLOB + source + SINGLE_WHITE_SPACE + AND_MARK
827+
+ SINGLE_WHITE_SPACE + destination,
828+
null);
829+
}
830+
} finally {
831+
incrementAbfsRenamePath();
823832
}
824833
}
825834

‎hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import org.apache.hadoop.util.StringUtils;
7171

7272
import static org.apache.commons.lang3.StringUtils.isEmpty;
73+
import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS;
7374
import static org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore.extractEtagHeader;
7475
import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.ACQUIRE_LEASE_ACTION;
7576
import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.APPEND_ACTION;
@@ -461,6 +462,8 @@ public void createNonRecursivePreCheck(Path parentPath,
461462
+ " because parent folder does not exist.");
462463
}
463464
throw ex;
465+
} finally {
466+
getAbfsCounters().incrementCounter(CALL_GET_FILE_STATUS, 1);
464467
}
465468
}
466469

‎hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,6 @@ public boolean execute() throws AzureBlobFileSystemException {
166166
result = renameInternal(src, dst);
167167
}
168168
} finally {
169-
getAbfsClient().incrementAbfsRenamePath();
170169
if (srcAbfsLease != null) {
171170
// If the operation is successful, cancel the timer and no need to release
172171
// the lease as rename on the blob-path has taken place.

‎hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemRename.java

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -327,12 +327,11 @@ public void testRenameNotFoundBlobToEmptyRoot() throws Exception {
327327
@Test
328328
public void testRenameBlobToDstWithColonInSourcePath() throws Exception {
329329
AzureBlobFileSystem fs = getFileSystem();
330-
assumeBlobServiceType();
331330
fs.create(new Path("/src:/file"));
332331
Assertions.assertThat(
333-
fs.rename(new Path("/src:"),
334-
new Path("/dst"))
335-
).isTrue();
332+
fs.rename(new Path("/src:"), new Path("/dst")))
333+
.describedAs("Rename should succeed")
334+
.isTrue();
336335
}
337336

338337
/**
@@ -349,9 +348,9 @@ public void testRenameWithColonInDestinationPath() throws Exception {
349348
AzureBlobFileSystem fs = getFileSystem();
350349
fs.create(new Path("/src"));
351350
Assertions.assertThat(
352-
fs.rename(new Path("/src"),
353-
new Path("/dst:"))
354-
).isTrue();
351+
fs.rename(new Path("/src"), new Path("/dst:")))
352+
.describedAs("Rename should succeed")
353+
.isTrue();
355354
}
356355

357356
@Test
@@ -364,26 +363,29 @@ public void testRenameWithColonInSourcePath() throws Exception {
364363
fs.create(new Path(sourceDirectory + "/Test:", fileName));
365364
// Rename from source to destination
366365
Assertions.assertThat(
367-
fs.rename(new Path(sourceDirectory),
368-
new Path(destinationDirectory))
369-
).isTrue();
366+
fs.rename(new Path(sourceDirectory), new Path(destinationDirectory)))
367+
.describedAs("Rename should succeed")
368+
.isTrue();
370369
Assertions.assertThat(
371-
fs.exists(new Path(sourceDirectory, fileName)))
370+
fs.exists(new Path(sourceDirectory, fileName)))
371+
.describedAs("Source directory should not exist after rename")
372372
.isFalse();
373373
Assertions.assertThat(
374374
fs.exists(new Path(destinationDirectory, fileName)))
375+
.describedAs("Destination directory should exist after rename")
375376
.isTrue();
376377

377378
// Rename from destination to source
378379
Assertions.assertThat(
379-
fs.rename(new Path(destinationDirectory),
380-
new Path(sourceDirectory))
381-
).isTrue();
380+
fs.rename(new Path(destinationDirectory), new Path(sourceDirectory)))
381+
.describedAs("Rename should succeed").isTrue();
382382
Assertions.assertThat(
383383
fs.exists(new Path(sourceDirectory, fileName)))
384+
.describedAs("Destination directory should exist after rename")
384385
.isTrue();
385386
Assertions.assertThat(
386387
fs.exists(new Path(destinationDirectory, fileName)))
388+
.describedAs("Source directory should not exist after rename")
387389
.isFalse();
388390
}
389391

@@ -1793,7 +1795,7 @@ private void validateRename(AzureBlobFileSystem fs, Path src, Path dst,
17931795
* Test the renaming of a directory with different parallelism configurations.
17941796
*/
17951797
@Test
1796-
public void testRenameDirWithDifferentParallelism() throws Exception {
1798+
public void testRenameDirWithDifferentParallelismConfig() throws Exception {
17971799
try (AzureBlobFileSystem currentFs = getFileSystem()) {
17981800
assumeBlobServiceType();
17991801
Path src = new Path("/hbase/A1/A2");
@@ -2073,7 +2075,8 @@ private void validateAtomicRenameKey(AbfsBlobClient abfsBlobClient, String path,
20732075
* @return file system
20742076
* @throws IOException in case of failure
20752077
*/
2076-
public AzureBlobFileSystem createJsonFile(Path path, Path renameJson) throws IOException {
2078+
public AzureBlobFileSystem createJsonFile(Path path, Path renameJson)
2079+
throws IOException {
20772080
final AzureBlobFileSystem fs = Mockito.spy(this.getFileSystem());
20782081
assumeBlobServiceType();
20792082
AzureBlobFileSystemStore store = Mockito.spy(fs.getAbfsStore());
@@ -2084,9 +2087,12 @@ public AzureBlobFileSystem createJsonFile(Path path, Path renameJson) throws IOE
20842087
fs.setWorkingDirectory(new Path(ROOT_PATH));
20852088
fs.create(new Path(path, "file.txt"));
20862089

2087-
AzureBlobFileSystemStore.VersionedFileStatus fileStatus = (AzureBlobFileSystemStore.VersionedFileStatus) fs.getFileStatus(path);
2090+
AzureBlobFileSystemStore.VersionedFileStatus fileStatus
2091+
= (AzureBlobFileSystemStore.VersionedFileStatus) fs.getFileStatus(path);
20882092

2089-
new RenameAtomicity(path, new Path("/hbase/test4"), renameJson, getTestTracingContext(fs, true), fileStatus.getEtag(), client)
2093+
new RenameAtomicity(path, new Path("/hbase/test4"),
2094+
renameJson, getTestTracingContext(fs, true),
2095+
fileStatus.getEtag(), client)
20902096
.preRename();
20912097

20922098
Assertions.assertThat(fs.exists(renameJson))
@@ -2106,7 +2112,7 @@ public AzureBlobFileSystem createJsonFile(Path path, Path renameJson) throws IOE
21062112
* @throws Exception if any error occurs during the test execution
21072113
*/
21082114
@Test
2109-
public void listCrashRecoveryWithSingleChildFolder() throws Exception {
2115+
public void testListCrashRecoveryWithSingleChildFolder() throws Exception {
21102116
AzureBlobFileSystem fs = null;
21112117
try {
21122118
Path path = new Path("/hbase/A1/A2");
@@ -2135,7 +2141,7 @@ public void listCrashRecoveryWithSingleChildFolder() throws Exception {
21352141
* @throws Exception if any error occurs during the test execution
21362142
*/
21372143
@Test
2138-
public void listCrashRecoveryWithMultipleChildFolder() throws Exception {
2144+
public void testListCrashRecoveryWithMultipleChildFolder() throws Exception {
21392145
AzureBlobFileSystem fs = null;
21402146
try {
21412147
Path path = new Path("/hbase/A1/A2");
@@ -2167,7 +2173,7 @@ public void listCrashRecoveryWithMultipleChildFolder() throws Exception {
21672173
* @throws Exception if any error occurs during the test execution
21682174
*/
21692175
@Test
2170-
public void listCrashRecoveryWithPendingJsonFile() throws Exception {
2176+
public void testListCrashRecoveryWithPendingJsonFile() throws Exception {
21712177
AzureBlobFileSystem fs = null;
21722178
try {
21732179
Path path = new Path("/hbase/A1/A2");
@@ -2200,7 +2206,7 @@ public void listCrashRecoveryWithPendingJsonFile() throws Exception {
22002206
* @throws Exception if any error occurs during the test execution
22012207
*/
22022208
@Test
2203-
public void listCrashRecoveryWithoutAnyPendingJsonFile() throws Exception {
2209+
public void testListCrashRecoveryWithoutAnyPendingJsonFile() throws Exception {
22042210
AzureBlobFileSystem fs = null;
22052211
try {
22062212
Path path = new Path("/hbase/A1/A2");
@@ -2233,7 +2239,7 @@ public void listCrashRecoveryWithoutAnyPendingJsonFile() throws Exception {
22332239
* @throws Exception if any error occurs during the test execution
22342240
*/
22352241
@Test
2236-
public void listCrashRecoveryWithPendingJsonDir() throws Exception {
2242+
public void testListCrashRecoveryWithPendingJsonDir() throws Exception {
22372243
try (AzureBlobFileSystem fs = Mockito.spy(this.getFileSystem())) {
22382244
assumeBlobServiceType();
22392245
AbfsBlobClient client = (AbfsBlobClient) addSpyHooksOnClient(fs);
@@ -2280,7 +2286,7 @@ public void listCrashRecoveryWithPendingJsonDir() throws Exception {
22802286
* @throws Exception if any error occurs during the test execution
22812287
*/
22822288
@Test
2283-
public void listCrashRecoveryWithMultipleJsonFile() throws Exception {
2289+
public void testListCrashRecoveryWithMultipleJsonFile() throws Exception {
22842290
AzureBlobFileSystem fs = null;
22852291
try {
22862292
Path path = new Path("/hbase/A1/A2");
@@ -2295,9 +2301,12 @@ public void listCrashRecoveryWithMultipleJsonFile() throws Exception {
22952301
fs.create(new Path(path2, "file3.txt"));
22962302

22972303
Path renameJson2 = new Path(path2.getParent(), path2.getName() + SUFFIX);
2298-
AzureBlobFileSystemStore.VersionedFileStatus fileStatus = (AzureBlobFileSystemStore.VersionedFileStatus) fs.getFileStatus(path2);
2304+
AzureBlobFileSystemStore.VersionedFileStatus fileStatus
2305+
= (AzureBlobFileSystemStore.VersionedFileStatus) fs.getFileStatus(path2);
22992306

2300-
new RenameAtomicity(path2, new Path("/hbase/test4"), renameJson2, getTestTracingContext(fs, true), fileStatus.getEtag(), client).preRename();
2307+
new RenameAtomicity(path2, new Path("/hbase/test4"),
2308+
renameJson2, getTestTracingContext(fs, true),
2309+
fileStatus.getEtag(), client).preRename();
23012310

23022311
fs.create(new Path(path, "file2.txt"));
23032312

@@ -2334,7 +2343,7 @@ public void listCrashRecoveryWithMultipleJsonFile() throws Exception {
23342343
* @throws Exception if any error occurs during the test execution
23352344
*/
23362345
@Test
2337-
public void getPathStatusWithPendingJsonFile() throws Exception {
2346+
public void testGetPathStatusWithPendingJsonFile() throws Exception {
23382347
AzureBlobFileSystem fs = null;
23392348
try {
23402349
Path path = new Path("/hbase/A1/A2");
@@ -2387,7 +2396,7 @@ public void getPathStatusWithPendingJsonFile() throws Exception {
23872396
* @throws Exception if any error occurs during the test execution
23882397
*/
23892398
@Test
2390-
public void getPathStatusWithoutPendingJsonFile() throws Exception {
2399+
public void testGetPathStatusWithoutPendingJsonFile() throws Exception {
23912400
try (AzureBlobFileSystem fs = Mockito.spy(this.getFileSystem())) {
23922401
assumeBlobServiceType();
23932402

@@ -2441,7 +2450,7 @@ public void getPathStatusWithoutPendingJsonFile() throws Exception {
24412450
* @throws Exception if any error occurs during the test execution
24422451
*/
24432452
@Test
2444-
public void getPathStatusWithPendingJsonDir() throws Exception {
2453+
public void testGetPathStatusWithPendingJsonDir() throws Exception {
24452454
try (AzureBlobFileSystem fs = Mockito.spy(this.getFileSystem())) {
24462455
assumeBlobServiceType();
24472456

@@ -2466,7 +2475,9 @@ public void getPathStatusWithPendingJsonDir() throws Exception {
24662475
conf.getClientCorrelationId(), fs.getFileSystemId(),
24672476
FSOperationType.GET_FILESTATUS, TracingHeaderFormat.ALL_ID_FORMAT, null);
24682477

2469-
AbfsHttpOperation abfsHttpOperation = client.getPathStatus(path.toUri().getPath(), true, tracingContext, null).getResult();
2478+
AbfsHttpOperation abfsHttpOperation
2479+
= client.getPathStatus(path.toUri().getPath(), true,
2480+
tracingContext, null).getResult();
24702481

24712482
Assertions.assertThat(abfsHttpOperation.getStatusCode())
24722483
.describedAs("Path should be found.")
@@ -2502,7 +2513,7 @@ public void getPathStatusWithPendingJsonDir() throws Exception {
25022513
* @throws Exception if any error occurs during the test execution
25032514
*/
25042515
@Test
2505-
public void eTagChangedDuringRename() throws Exception {
2516+
public void testETagChangedDuringRename() throws Exception {
25062517
AzureBlobFileSystem fs = null;
25072518
try {
25082519
assumeBlobServiceType();

0 commit comments

Comments
 (0)