Skip to content
This repository was archived by the owner on Jul 19, 2024. It is now read-only.

Commit 7065758

Browse files
jaschrep-msftrickle-msft
jaschrep-msft
authored and
rickle-msft
committed
Addressed comments
1 parent 5f4bcc4 commit 7065758

File tree

8 files changed

+126
-101
lines changed

8 files changed

+126
-101
lines changed

‎microsoft-azure-storage-test/src/com/microsoft/azure/storage/blob/CloudBlobClientTests.java‎

Lines changed: 31 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,7 @@ public void testBatchBlobDelete() throws Exception {
408408

409409
// execute batch
410410

411-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchDeleteOp);
411+
Map<CloudBlob, Void> responses = container.getServiceClient().executeBatch(batchDeleteOp);
412412

413413
// validate
414414

@@ -417,8 +417,8 @@ public void testBatchBlobDelete() throws Exception {
417417
}
418418

419419
int numResponses = 0;
420-
for (Map.Entry<CloudBlob, Void> response : responses) {
421-
assertEquals(blobs.get(numResponses), response.getKey());
420+
for (Map.Entry<CloudBlob, Void> response : responses.entrySet()) {
421+
assertTrue(blobs.contains(response.getKey()));
422422
assertNull(response.getValue()); // CloudBlob::delete() returns Void (null), so all batch responses should be null
423423
numResponses++;
424424
}
@@ -438,7 +438,7 @@ public void testBatchBlobDeleteNoRequests() throws Exception {
438438

439439
// execute batch
440440

441-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchDeleteOp); //throws
441+
Map<CloudBlob, Void> responses = container.getServiceClient().executeBatch(batchDeleteOp); //throws
442442
}
443443

444444
@Test
@@ -458,7 +458,6 @@ public void testBatchBlobDeleteMixedRequests() throws Exception {
458458
for (int i = 0; i < BLOBS; i++) {
459459
CloudBlockBlob blob = container.getBlockBlobReference("testblob_" + UUID.randomUUID());
460460

461-
// bad requests first; displays that good responses come first regardless of order of request
462461
if (i >= BAD_REQUESTS) {
463462
blob.uploadText("content");
464463
}
@@ -469,33 +468,20 @@ public void testBatchBlobDeleteMixedRequests() throws Exception {
469468

470469
// execute batch
471470

472-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchDeleteOp);
473-
474-
// validate
475-
476-
// good deletes are successful
477-
for (CloudBlob blob : blobs) {
478-
assertFalse(blob.exists());
479-
}
480-
481-
// manually iterate to show we get every result we expect
482-
Iterator<Map.Entry<CloudBlob, Void>> it = responses.iterator();
483-
for (int i = 0; i < BLOBS - BAD_REQUESTS; i++) {
484-
assertTrue(it.hasNext());
485-
assertTrue(blobs.contains(it.next().getKey()));
486-
}
487-
471+
Map<CloudBlob, Void> responses;
488472
boolean threw = true;
489473
try {
490-
it.hasNext(); // throws
474+
responses = container.getServiceClient().executeBatch(batchDeleteOp);
491475
threw = false;
492476
}
477+
478+
// validate
479+
493480
catch (BatchException e) {
494-
assertEquals(BAD_REQUESTS, e.size());
495-
for (Map.Entry<StorageException, Object> entry : e.entrySet()) {
496-
assertNotNull(entry.getKey());
497-
// unnecessary cast for Collection::contains(), but shows the value must usually be casted
498-
assertTrue(blobs.contains((CloudBlob)entry.getValue()));
481+
// good deletes are successful
482+
for (CloudBlob blob : blobs) {
483+
assertFalse(blob.exists());
484+
assertTrue(e.getSuccessfulResponses().keySet().contains(blob) || e.getExceptions().keySet().contains(blob));
499485
}
500486
}
501487
finally {
@@ -525,7 +511,7 @@ public void testBatchBlobSetTier() throws Exception {
525511

526512
// execute batch
527513

528-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchTierOp);
514+
Map<CloudBlob, Void> responses = container.getServiceClient().executeBatch(batchTierOp);
529515

530516
// validate
531517

@@ -535,8 +521,8 @@ public void testBatchBlobSetTier() throws Exception {
535521
}
536522

537523
int numResponses = 0;
538-
for (Map.Entry<CloudBlob, Void> response : responses) {
539-
assertEquals(blobs.get(numResponses), response.getKey());
524+
for (Map.Entry<CloudBlob, Void> response : responses.entrySet()) {
525+
assertTrue(blobs.contains(response.getKey()));
540526
assertNull(response.getValue()); // CloudBlob::setTier() returns Void (null), so all batch responses should be null
541527
numResponses++;
542528
}
@@ -556,7 +542,7 @@ public void testBatchBlobSetTierNoRequests() throws Exception {
556542

557543
// execute batch
558544

559-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchDeleteOp); //throws
545+
Map<CloudBlob, Void> responses = container.getServiceClient().executeBatch(batchDeleteOp); //throws
560546
}
561547

562548
@Test
@@ -571,7 +557,7 @@ public void testBatchBlobSetTierMixedRequests() throws Exception {
571557
container.createIfNotExists();
572558

573559
List<CloudBlob> blobs = new ArrayList<>();
574-
BlobSetTierBatchOperation batchDeleteOp = new BlobSetTierBatchOperation();
560+
BlobSetTierBatchOperation batchSetTierOp = new BlobSetTierBatchOperation();
575561

576562
for (int i = 0; i < BLOBS; i++) {
577563
CloudBlockBlob blob = container.getBlockBlobReference("testblob_" + UUID.randomUUID());
@@ -582,41 +568,28 @@ public void testBatchBlobSetTierMixedRequests() throws Exception {
582568
}
583569

584570
blobs.add(blob);
585-
batchDeleteOp.addSubOperation(blob, StandardBlobTier.HOT);
571+
batchSetTierOp.addSubOperation(blob, StandardBlobTier.HOT);
586572
}
587573

588574
// execute batch
589575

590-
Iterable<Map.Entry<CloudBlob, Void>> responses = container.getServiceClient().executeBatch(batchDeleteOp);
591-
592-
// validate
593-
594-
// good deletes are successful
595-
for (CloudBlob blob : blobs) {
596-
if (blob.exists()) {
597-
blob.downloadAttributes();
598-
assertEquals(StandardBlobTier.HOT, blob.getProperties().getStandardBlobTier());
599-
}
600-
}
601-
602-
// manually iterate to show we get every result we expect
603-
Iterator<Map.Entry<CloudBlob, Void>> it = responses.iterator();
604-
for (int i = 0; i < BLOBS - BAD_REQUESTS; i++) {
605-
assertTrue(it.hasNext());
606-
assertTrue(blobs.contains(it.next().getKey()));
607-
}
608-
576+
Map<CloudBlob, Void> responses;
609577
boolean threw = true;
610578
try {
611-
it.hasNext(); // throws
579+
responses = container.getServiceClient().executeBatch(batchSetTierOp);
612580
threw = false;
613581
}
582+
583+
// validate
584+
614585
catch (BatchException e) {
615-
assertEquals(BAD_REQUESTS, e.size());
616-
for (Map.Entry<StorageException, Object> entry : e.entrySet()) {
617-
assertNotNull(entry.getKey());
618-
// unnecessary cast for Collection::contains(), but shows the value must usually be casted
619-
assertTrue(blobs.contains((CloudBlob)entry.getValue()));
586+
// good deletes are successful
587+
for (CloudBlob blob : blobs) {
588+
if (blob.exists()) {
589+
blob.downloadAttributes();
590+
assertEquals(StandardBlobTier.HOT, blob.getProperties().getStandardBlobTier());
591+
}
592+
assertTrue(e.getSuccessfulResponses().keySet().contains(blob) || e.getExceptions().keySet().contains(blob));
620593
}
621594
}
622595
finally {

‎microsoft-azure-storage/src/com/microsoft/azure/storage/BatchException.java‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,33 +10,33 @@
1010
/**
1111
* Exception for when one or more sub-requests within a batch request fail. This exception is a map of the
1212
* {@link StorageException}s to the parent objects of the sub-request. Extensions of {@link Throwable} cannot use
13-
* generics, so this class uses data several structures of type Object. Since only groups of the same request type can
13+
* generics, so this class uses data several structures with wildcards. Since only groups of the same request type can
1414
* be batched together, the batch caller will know the intended type in context, and can safely cast the result.
1515
*/
1616
public class BatchException extends StorageException {
1717

1818
/**
19-
* Maps a successful response within a batch to the parent object of the request (i.e. Void maps to the CloudBlob
20-
* object used for a successful delete request, as the normal delete() method on that object returns Void in its
19+
* Maps the parent object of the request to a successful response within a batch (i.e. the CloudBlob object used for
20+
* a successful delete request maps to Void, as the normal delete() method on that object returns Void in its
2121
* StorageRequest implementation).
2222
*/
23-
private final Map<Object, Object> successfulResponses;
23+
private final Map<?, ?> successfulResponses;
2424

2525
/**
26-
* Maps a failed sub-request to the parent object of the request (i.e. StorageException from a delete blob request
26+
* Maps the parent object of the request to a failed sub-request (i.e. StorageException from a delete blob request
2727
* maps to the CloudBlob object used for the delete).
2828
*/
29-
private final Map<StorageException, Object> exceptions;
29+
private final Map<?, StorageException> exceptions;
3030

3131

32-
BatchException(Map<Object, Object> successfulResponses, Map<BatchSubResponse, Object> failedResponses) {
32+
BatchException(Map<?, ?> successfulResponses, Map<?, BatchSubResponse> failedResponses) {
3333
super("Batch exception", "One ore more requests in a batch operation failed", null);
3434

35-
Map<StorageException, Object> exceptions = new HashMap<>(failedResponses.size());
36-
for (Map.Entry<BatchSubResponse, Object> response : failedResponses.entrySet()) {
35+
Map<Object, StorageException> exceptions = new HashMap<>(failedResponses.size());
36+
for (Map.Entry<?, BatchSubResponse> response : failedResponses.entrySet()) {
3737

3838
StringBuilder builder = new StringBuilder();
39-
try (Reader reader = new BufferedReader(new InputStreamReader(response.getKey().getBody(), StandardCharsets.UTF_8))) {
39+
try (Reader reader = new BufferedReader(new InputStreamReader(response.getValue().getBody(), StandardCharsets.UTF_8))) {
4040
int character;
4141
while ((character = reader.read()) != -1) {
4242
builder.append((char)character);
@@ -46,20 +46,20 @@ public class BatchException extends StorageException {
4646
}
4747

4848
exceptions.put(
49-
new StorageException(response.getKey().getStatusMessage(), builder.toString(),
50-
response.getKey().getStatusCode(), null, null),
51-
response.getValue());
49+
response.getKey(),
50+
new StorageException(response.getValue().getStatusMessage(), builder.toString(),
51+
response.getValue().getStatusCode(), null, null));
5252
}
5353

5454
this.successfulResponses = Collections.unmodifiableMap(successfulResponses);
5555
this.exceptions = Collections.unmodifiableMap(exceptions);
5656
}
5757

58-
public Map<Object, Object> getSuccessfulResponses() {
58+
public Map<?, ?> getSuccessfulResponses() {
5959
return successfulResponses;
6060
}
6161

62-
public Map<StorageException, Object> getExceptions() {
62+
public Map<?, StorageException> getExceptions() {
6363
return exceptions;
6464
}
6565
}

‎microsoft-azure-storage/src/com/microsoft/azure/storage/BatchOperation.java‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,14 @@
2020
* @param <P>
2121
* The type of the parent object making the REST call.
2222
* @param <R>
23+
* The return type of the individual requests on the batch.
2324
*/
2425
public abstract class BatchOperation <C extends ServiceClient, P, R> implements Iterable<Map.Entry<StorageRequest<C, P, R>, P>> {
2526

26-
private static String HTTP_LINE_ENDING = "\r\n";
27+
private static final String HTTP_LINE_ENDING = "\r\n";
28+
private static final String HTTP_MIXED_MULTIPART_CONTENT_TYPE = "multipart/mixed";
29+
private static final String HTTP_MIXED_MULTIPART_DELIMITER_DEFINITION = "boundary=";
30+
private static final String STORAGE_BATCH_DELIMITER_PREFIX = "batch_";
2731

2832
private final Map<StorageRequest<C, P, R>, P> subOperations = new LinkedHashMap<>(); // maintains iteration order
2933

@@ -87,7 +91,11 @@ public HttpURLConnection buildRequest(C client, BatchOperation<C, P, R> parentOb
8791

8892
@Override
8993
public void setHeaders(HttpURLConnection connection, BatchOperation<C, P, R> parentObject, OperationContext context) {
90-
connection.setRequestProperty(Constants.HeaderConstants.CONTENT_TYPE, "multipart/mixed; boundary=batch_" + parentObject.getBatchId());
94+
connection.setRequestProperty(
95+
Constants.HeaderConstants.CONTENT_TYPE,
96+
Utility.stringJoin("; ",
97+
HTTP_MIXED_MULTIPART_CONTENT_TYPE,
98+
HTTP_MIXED_MULTIPART_DELIMITER_DEFINITION + STORAGE_BATCH_DELIMITER_PREFIX + parentObject.getBatchId()));
9199
}
92100

93101
@Override
@@ -100,7 +108,7 @@ public void signRequest(HttpURLConnection connection, C client, OperationContext
100108
public Map<P, R> preProcessResponse(BatchOperation<C, P, R> parentObject, C client,
101109
OperationContext context) throws Exception {
102110
if (this.getResult().getStatusCode() != HttpURLConnection.HTTP_ACCEPTED) {
103-
throw new StorageException(this.getResult().getErrorCode(), this.getResult().getStatusMessage(), null);
111+
this.setNonExceptionedRetryableFailure(true);
104112
}
105113

106114
return null;
@@ -120,13 +128,19 @@ public Map<P, R> postProcessResponse(HttpURLConnection connection, BatchOperatio
120128

121129
final List<BatchSubResponse> parsedResponses = parseBatchBody(
122130
responseBuffer.toByteArray(),
123-
connection.getHeaderField(Constants.HeaderConstants.CONTENT_TYPE).split("boundary=")[1]);
131+
connection.getHeaderField(Constants.HeaderConstants.CONTENT_TYPE).split(HTTP_MIXED_MULTIPART_DELIMITER_DEFINITION)[1]);
124132

125-
assertBatchSuccess(parsedResponses);
133+
throwIfUberRequestFails(parsedResponses);
126134

127135
final Map<P, R> successfulResponses = new HashMap<>();
128136
final Map<P, BatchSubResponse> failedResponses = new HashMap<>();
137+
sortResponses(parsedResponses, successfulResponses, failedResponses);
129138

139+
if (!failedResponses.isEmpty()) {
140+
throw new BatchException(successfulResponses, failedResponses);
141+
}
142+
143+
return successfulResponses;
130144
}
131145
};
132146
}
@@ -176,14 +190,25 @@ private void sortResponses(
176190
* @throws StorageException
177191
* If the body contained a single response detailing that the batch failed.
178192
*/
179-
private void assertBatchSuccess(List<BatchSubResponse> parsedResponses) throws StorageException {
193+
private void throwIfUberRequestFails(List<BatchSubResponse> parsedResponses) throws StorageException {
180194

195+
// Failure results in 1 and only 1 response. If this is not the case, we're successful.
181196
if (parsedResponses.size() != 1) {
182197
return;
183198
}
184199

185200
BatchSubResponse subResponse = parsedResponses.get(0);
186201

202+
// If single response is successful, we successfully batched one request.
203+
if (subResponse.getStatusCode() / 100 == 2) {
204+
return;
205+
}
206+
207+
/* If the user batched one request and it failed, we do not have a method of determining whether the overall
208+
* batch failed, or if the single subrequest failed. Either way, there was not a single success, so throw as if
209+
* the batch failed.
210+
*/
211+
187212
ByteArrayOutputStream responseBuffer = new ByteArrayOutputStream();
188213
try {
189214
int bytesRead;
@@ -197,7 +222,6 @@ private void assertBatchSuccess(List<BatchSubResponse> parsedResponses) throws S
197222
throw new RuntimeException(e);
198223
}
199224

200-
201225
throw new StorageException(subResponse.getStatusMessage(), responseBuffer.toString(), subResponse.getStatusCode(), null, null);
202226
}
203227

‎microsoft-azure-storage/src/com/microsoft/azure/storage/BatchSubResponse.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,31 +19,31 @@ public int getStatusCode() {
1919
return statusCode;
2020
}
2121

22-
public void setStatusCode(int statusCode) {
22+
void setStatusCode(int statusCode) {
2323
this.statusCode = statusCode;
2424
}
2525

2626
public String getStatusMessage() {
2727
return status;
2828
}
2929

30-
public void setStatusMessage(String status) {
30+
void setStatusMessage(String status) {
3131
this.status = status;
3232
}
3333

3434
public Map<String, String> getHeaders() {
3535
return headers;
3636
}
3737

38-
public void setHeaders(Map<String, String> headers) {
38+
void setHeaders(Map<String, String> headers) {
3939
this.headers = headers;
4040
}
4141

4242
public InputStream getBody() {
4343
return body;
4444
}
4545

46-
public void setBody(InputStream body) {
46+
void setBody(InputStream body) {
4747
this.body = body;
4848
}
4949
}

‎microsoft-azure-storage/src/com/microsoft/azure/storage/blob/BlobBatchOperation.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
public abstract class BlobBatchOperation<P, R> extends BatchOperation<CloudBlobClient, P, R> {
99

10-
final Iterable<Map.Entry<P, R>> execute(CloudBlobClient client, BlobRequestOptions requestOptions, OperationContext operationContext)
10+
final Map<P, R> execute(CloudBlobClient client, BlobRequestOptions requestOptions, OperationContext operationContext)
1111
throws StorageException {
1212

1313
if (operationContext == null) {

0 commit comments

Comments
 (0)