Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

cleanup(batcher): removing the non-applicable TODOs comments #834

Merged
merged 2 commits into from Dec 27, 2019
Merged

cleanup(batcher): removing the non-applicable TODOs comments #834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1dfefca
fix(batcher): fix logged messge when exception occurs post processing
rahulKQL Dec 16, 2019
8f34639
removed unnecessary batcher message changes, Also reverted the Batche…
rahulKQL Dec 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java
View file
Open in desktop
Expand Up @@ -177,7 +177,7 @@ public void onSuccess(ResponseT response) {
@Override
public void onFailure(Throwable throwable) {
try {
accumulatedBatch.onBatchFailure(throwable);
accumulatedBatch.onBatchFailure(throwable, false);
} finally {
onBatchCompletion();
}
Expand Down Expand Up @@ -256,26 +256,29 @@ void add(ElementT element, SettableApiFuture<ElementResultT> result) {
byteCounter += descriptor.countBytes(element);
}

// TODO: Ensure that all results are resolved in case the descriptor that causes it to
// process all results or throw an exception during processing
void onBatchSuccess(ResponseT response) {
try {
descriptor.splitResponse(response, results);
batcherStats.recordBatchElementsCompletion(results);
batcherStats.recordPartialBatchFailure(results);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the old method is more appropriate: you are notifying the stats object that all of the passed in elements are resolved. The fact that the stats object counts only the failures is its own business

} catch (Exception ex) {
onBatchFailure(ex);
onBatchFailure(ex, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very hard to understand what true means in this context and why it's different from the invocation in sendOutstanding(). If we go down this route, then we should define a separate method onBatchResponseProcessingFailure or something.

}
}

void onBatchFailure(Throwable throwable) {
void onBatchFailure(Throwable throwable, boolean isPartialFailed) {
try {
descriptor.splitException(throwable, results);
} catch (Exception ex) {
for (SettableApiFuture<ElementResultT> result : results) {
result.setException(ex);
}
}
batcherStats.recordBatchFailure(throwable);

if (isPartialFailed) {
batcherStats.recordPartialBatchFailure(results);
} else {
batcherStats.recordBatchFailure(throwable);
}
}

boolean isEmpty() {
Expand Down
2 changes: 1 addition & 1 deletion gax/src/main/java/com/google/api/gax/batching/BatcherStats.java
View file
Open in desktop
Expand Up @@ -82,7 +82,7 @@ synchronized void recordBatchFailure(Throwable throwable) {
* <p>Note: This method aggregates all the subclasses of {@link ApiException} under ApiException
* using the {@link Code status codes} and its number of occurrences.
*/
synchronized <T extends ApiFuture> void recordBatchElementsCompletion(
synchronized <T extends ApiFuture> void recordPartialBatchFailure(
List<T> batchElementResultFutures) {
boolean isRequestPartiallyFailed = false;
for (final ApiFuture elementResult : batchElementResultFutures) {
Expand Down
15 changes: 8 additions & 7 deletions gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java
View file
Open in desktop
Expand Up @@ -262,8 +262,8 @@ public void splitResponse(
assertThat(actualError)
.hasMessageThat()
.contains(
"Batching finished with 1 batches failed to apply due to: 1 RuntimeException and 0 "
+ "partial failures.");
"Batching finished with 1 partial failures. The 1 partial failures contained 1 "
+ "entries that failed with: 1 RuntimeException.");
}

/** Resolves future results when {@link BatchingDescriptor#splitException} throws exception */
Expand Down Expand Up @@ -509,7 +509,6 @@ public void splitResponse(
+ "3 entries that failed with: 3 ArithmeticException.");
}

// TODO(rahulkql): fix this test with follow up PR related to exception in splitResponse.
@Test
public void testPartialFailureInResultProcessing() throws Exception {
final Queue<RuntimeException> queue = Queues.newArrayBlockingQueue(3);
Expand All @@ -523,6 +522,7 @@ public void testPartialFailureInResultProcessing() throws Exception {
@Override
public void splitResponse(
List<Integer> batchResponse, List<SettableApiFuture<Integer>> batch) {
// Mocking the failure during batching response processing
throw queue.poll();
}
};
Expand Down Expand Up @@ -553,11 +553,12 @@ public void splitResponse(
assertThat(actualError).isInstanceOf(BatchingException.class);
assertThat(actualError)
.hasMessageThat()
.contains("Batching finished with 3 batches failed to apply due to:");
.contains(
"Batching finished with 3 partial failures. The 3 partial failures contained 6 "
+ "entries that failed with:");
assertThat(actualError).hasMessageThat().contains("1 NullPointerException");
assertThat(actualError).hasMessageThat().contains("1 RuntimeException");
assertThat(actualError).hasMessageThat().contains("1 ArithmeticException");
assertThat(actualError).hasMessageThat().contains(" and 0 partial failures.");
assertThat(actualError).hasMessageThat().contains("2 RuntimeException");
assertThat(actualError).hasMessageThat().contains("3 ArithmeticException");
}

/**
Expand Down
6 changes: 3 additions & 3 deletions gax/src/test/java/com/google/api/gax/batching/BatcherStatsTest.java
View file
Open in desktop
Expand Up @@ -70,11 +70,11 @@ public void testRequestFailuresOnly() {
@Test
public void testEntryFailureOnly() {
BatcherStats batcherStats = new BatcherStats();
batcherStats.recordBatchElementsCompletion(
batcherStats.recordPartialBatchFailure(
ImmutableList.of(
ApiFutures.immediateFailedFuture(new IllegalStateException("local element failure"))));

batcherStats.recordBatchElementsCompletion(
batcherStats.recordPartialBatchFailure(
ImmutableList.of(
ApiFutures.immediateFailedFuture(
ApiExceptionFactory.createException(
Expand All @@ -94,7 +94,7 @@ public void testRequestAndEntryFailures() {
BatcherStats batcherStats = new BatcherStats();

batcherStats.recordBatchFailure(new RuntimeException("Batch failure"));
batcherStats.recordBatchElementsCompletion(
batcherStats.recordPartialBatchFailure(
ImmutableList.of(
ApiFutures.immediateFailedFuture(
ApiExceptionFactory.createException(
Expand Down