ctubbsii commented on a change in pull request #2337:
URL: https://github.com/apache/accumulo/pull/2337#discussion_r741634717
##########
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:
@andrewglowacki wrote:
> The Throwable is passed through to the client via updateUnknownErrors
where it is re-thrown. The client can decide to halt the JVM if they so choose.
From what I see, only the lastUnknownError is preserved. This might just get
logged and never sent back to the calling client code. The Error can easily get
suppressed by a more recent and less serious exception, and the serious Error
go overlooked indefinitely.
@keith-turner wrote:
> it seems reasonable that the library should make a best effort attempt to
get any errors that occur in that thread back to the user code
I probably agree that is a reasonable goal, but I'm not convinced the
proposed change does that. I only had time for a quick code review, and it
looked like this change was more likely to result in the Errors being *less*
noticeable to users than *more* noticeable. However, it's not completely
suppressed, as I had originally thought. There is a chance the Error will make
it back to the user code via a MutationsRejectedException. I don't think it's
quite as "failsafe" as killing the JVM or triggering a user-defined
UncaughtExceptionHandler, but it might be the best option for 1.10.
I'd feel more comfortable about this if I knew that the Error couldn't be
completely clobbered by a less severe Exception and that it definitely would
make it back to the user code. Perhaps something can be done with
[`addSuppressed`](https://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html#addSuppressed(java.lang.Throwable))
?
> It seems like the argument against this is that it may not work therefore
do not even make the attempt.
I don't recognize my position in your portrayal here.
##########
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:
@andrewglowacki wrote:
> The Throwable is passed through to the client via updateUnknownErrors
where it is re-thrown. The client can decide to halt the JVM if they so choose.
From what I see, only the lastUnknownError is preserved. This might just get
logged and never sent back to the calling client code. The Error can easily get
suppressed by a more recent and less serious exception, and the serious Error
go overlooked indefinitely.
@keith-turner wrote:
> it seems reasonable that the library should make a best effort attempt to
get any errors that occur in that thread back to the user code
I probably agree that is a reasonable goal, but I'm not convinced the
proposed change does that. I only had time for a quick code review, and it
looked like this change was more likely to result in the Errors being *less*
noticeable to users than *more* noticeable. However, it's not completely
suppressed, as I had originally thought. There is a chance the Error will make
it back to the user code via a MutationsRejectedException. I don't think it's
quite as "failsafe" as killing the JVM or triggering a user-defined
UncaughtExceptionHandler, but it might be the best option for 1.10.
I'd feel more comfortable about this if I knew that the Error couldn't be
completely clobbered by a less severe Exception and that it definitely would
make it back to the user code. Perhaps something can be done with
[`addSuppressed`](https://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html#addSuppressed(java.lang.Throwable))
?
> It seems like the argument against this is that it may not work therefore
do not even make the attempt.
I don't recognize my position in your portrayal here.
--
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]
