Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Dec 27, 2019

Conversation

rahulKQL
Copy link
Contributor

@rahulKQL rahulKQL commented Dec 16, 2019

This commit simply removes TODO comments from BatcherImpl.java & BatcherImplTest.java, As the task mentioned on TODOs are not needed now(Please refer below conversation).

This commit addresses logged message while batch post
processing.

Currently, if a batch is successful and some exception
occurs during processing then the Batcher generates an exception message
with "Batching finished with 3 batches failed" which is not an accurate
message.

After this commit, for the same scenario, the message will be
logged with "Batching finished with 3 partial failures...".
@rahulKQL rahulKQL requested a review from igorbernstein2 Dec 16, 2019
@googlebot googlebot added the cla: yes label Dec 16, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 16, 2019

Codecov Report

Merging #834 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #834   +/-   ##
=========================================
  Coverage     78.72%   78.72%           
  Complexity     1146     1146           
=========================================
  Files           202      202           
  Lines          5099     5099           
  Branches        405      405           
=========================================
  Hits           4014     4014           
  Misses          912      912           
  Partials        173      173
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/api/gax/batching/BatcherImpl.java 98% <ø> (ø) 16 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68ae22d...8f34639. Read the comment docs.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

I think this needs a bit more work.

I'm not sure I agree with the description of the problem

Currently, if a batch is successful and some exception occurs during processing then the Batcher generates an exception message with "Batching finished with 3 batches failed" which is not an accurate message.

Wouldn't the message be: Batching finished with 1 batches failed?
Tracing the code:
sendOutstanding onSuccess -> Batch#onBatchSuccess -> BatchingDescriptor#splitResponse()
which throws an error, the Error is passed to
Batch#onBatchFailure -> BatcherStats#recordBatchFailure,
which will only increment a single error count

So I'm guessing your intention here is improve the error message be specifically directed to each entry that wasn't processed. If that's the case, a more appropriate approach would be to pass both the result list & the batch level error to stats. Then stats can check if any entries succeeded and decide on the appropriate counting.

However, I don't think this optimization is needed. BatchingDescriptors should never throw an exception, if they do, then it's a bug in the client implementation. So unless I'm misunderstanding the issue, I think we should merge this

void onBatchSuccess(ResponseT response) {
try {
descriptor.splitResponse(response, results);
batcherStats.recordBatchElementsCompletion(results);
batcherStats.recordPartialBatchFailure(results);
Copy link
Collaborator

@igorbernstein2 igorbernstein2 Dec 17, 2019

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
Collaborator

@igorbernstein2 igorbernstein2 Dec 17, 2019

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.

@igorbernstein2 igorbernstein2 added the do not merge label Dec 17, 2019
@rahulKQL
Copy link
Contributor Author

@rahulKQL rahulKQL commented Dec 17, 2019

Thanks for the detailed reply.

So I'm guessing your intention here is improve the error message be specifically directed to each entry that wasn't processed. If that's the case, a more appropriate approach would be to pass both the result list & the batch level error to stats. Then stats can check if any entries succeeded and decide on the appropriate counting.

Yes, I meant to improve the error message for partially failed batches, Currently, number of partially failed elements cannot be known in case of exception occurs in BatcherDescriptor#splitResponse.

However, I don't think this optimization is needed. BatchingDescriptors should never throw an exception, if they do, then it's a bug in the client implementation.

I agree with your point, I wasn't sure about that before.

I think I will revert these changes along with BatcherStats's method name and probably remove the TODOs.

…rStats method name to `recordBatchElementsCompletion()`.
@rahulKQL
Copy link
Contributor Author

@rahulKQL rahulKQL commented Dec 18, 2019

@igorbernstein2 Have reverted both error message and method renaming.

The TODOs were for the same(to improve error message) reason so deleting these now. PTAL!

@rahulKQL rahulKQL requested a review from igorbernstein2 Dec 18, 2019
@igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Dec 18, 2019

Please update the PR title & description

@rahulKQL rahulKQL changed the title fix(batcher): fix logged message when exception occurs post processing cleanup(batcher): removing the non-applicable TODOs comments Dec 18, 2019
@rahulKQL rahulKQL removed the do not merge label Dec 18, 2019
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

LGTM

@igorbernstein2 igorbernstein2 merged commit bbefe87 into googleapis:master Dec 27, 2019
5 checks passed
@kolea2 kolea2 mentioned this pull request Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants