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

[rahulKQL]

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).

[codecov]

Codecov Report

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

@@            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.

[igorbernstein2]

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

[igorbernstein2]

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

[igorbernstein2]

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.

[rahulKQL]

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.

[rahulKQL]

@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!

[igorbernstein2]

Please update the PR title & description

[igorbernstein2]

LGTM