ctubbsii commented on a change in pull request #2337:
URL: https://github.com/apache/accumulo/pull/2337#discussion_r742551082
##########
File path:
core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java
##########
@@ -763,7 +763,7 @@ public void run() {
log.trace("{} - binning {} mutations",
Thread.currentThread().getName(),
mutationsToSend.size());
addMutations(mutationsToSend);
- } catch (Exception e) {
+ } catch (Throwable e) {
updateUnknownErrors("Error processing mutation set", e);
Review comment:
<details>
<summary>@keith-turner No worries.</summary>
Reading back through #2331, I can see how my strongly-worded opinions there
would prime someone into thinking I'd be against attempting to solve this here.
While I may have some strong philosophical opinions on how to handle these
kinds of Errors in general, I just want to be clear that I'm not opposed to
attempting a practical workaround here.
</details>
@andrewglowacki I think you're right that there are still other cases where
they wouldn't be returned. Problems queued by threads after the user got a
previous problem is one case (though these might be seen later, during a
`bw.flush()` or `bw.close()`). Another case might be if a `RuntimeException` or
`Error` were to be thrown from the outer thread (main thread?) that fell
through to the calling code and prevented the queued problems from being sent
back.
Additionally, merely adding suppressed mutations might cause the same
problems to reported multiple times, unless we clear them after they get sent
to the calling code. For example, if `bw.addMutation(m)` were wrapped with
try/catch block instead of a single catch clause at the end of a
try-with-resources block for the `BatchWriter`. However, clearing problems
isn't great because, currently, the failure state is never reset for the BW
when problem occurred. So, it appears that the current code could throw the
last error multiple times. For example, `addMutation` could throw it, and then,
since it's not reset, it could throw again at `flush` and/or `close`. If we
clear the queue of problems to avoid the same problems reported multiple times,
that might cause the second throw of `MutationsRejectedException` to not have
an actual problem to wrap (because it might have been cleared, even though
`somethingFailed` is still `true`). I'm not really sure the preferred behavior
here. It's m
ore complicated than it seems at first glance, because we're queuing up these
problems instead of handling them immediately. It's uncertain how best to
handle it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
