[GitHub] [accumulo] dlmarion opened a new issue, #3395: 2.1 still catching Error's

["dlmarion (via GitHub)" <gi...@apache.org>]

dlmarion opened a new issue, #3395:
URL: https://github.com/apache/accumulo/issues/3395

   `grep -R Error *  | grep java | grep -i catch | grep -v AssertionError` on the source yields:
   
   ```
   core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java:    } catch (LocalityGroupConfigurationError e) {
   core/src/main/java/org/apache/accumulo/core/util/LocalityGroupUtil.java:    } catch (LocalityGroupConfigurationError | RuntimeException e) {
   core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java:      } catch (LocalityGroupConfigurationError | RuntimeException e) {
   core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:      } catch (LocalityGroupConfigurationError | RuntimeException e) {
   core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:      } catch (LocalityGroupConfigurationError | RuntimeException e) {
   core/src/test/java/org/apache/accumulo/core/util/LocalityGroupUtilTest.java:    } catch (LocalityGroupConfigurationError err) {
   server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:      } catch (Error e) {
   server/base/src/main/java/org/apache/accumulo/server/compaction/FileCompactor.java:    } catch (LocalityGroupConfigurationError e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java:    } catch (Exception | UnsatisfiedLinkError e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java:      } catch (Exception | UnsatisfiedLinkError e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:    } catch (Exception | Error e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/MinorCompactor.java:        } catch (IOException | UnsatisfiedLinkError e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/MinorCompactor.java:        } catch (RuntimeException | NoClassDefFoundError e) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:          } catch (IOException | FSError ex) {
   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:        } catch (IOException | FSError ex) {
   ```
   
   Need to evaluate these to see if we should fix these.


-- 
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: notifications-unsubscribe@accumulo.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] ctubbsii commented on issue #3395: 2.1 still catching Error's

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on issue #3395:
URL: https://github.com/apache/accumulo/issues/3395#issuecomment-1545048610

   Some triage:
   
   **Stuff that's okay as is, and doesn't need to change**
   
   * The UnsatisfiedLinkError in the NativeMapLoader is okay. It's specific handling of the expected error when we try to load native maps and detect that they aren't available. We need to handle it so we can try to load it a different way, because there's multiple ways to load them.
   
   * FSError is being caught to retry when writing walogs or minor compaction files. This one is stupid, because these aren't actually Errors. These are created by Hadoop, and are wrapping IOExceptions when it encounters one with the native file system. Hadoop is needlessly upgrading their severity by wrapping IOExceptions as FSError, and we're just treating them the same as any other IOException. They should have just extended IOException and called it something better.
   
   **Stuff that's okay, but maybe could be improved**
   
   * LocalityGroupConfigurationError is misnamed, at a minimum. It's a regular Exception, and not public API. It can be renamed. In most places, we're treating it like an IllegalArgumentException, and are either wrapping it as such, or checking it alongside RuntimeExceptions, which would catch it if it were one. It should probably be renamed and made to extend that. Would need to check to make sure we don't break any checks that are implicitly catching it as an AcumuloException (which it currently extends).
   
   **Stuff that needs further investigation**
   
   * TServerUtils is catching Error to Halt with a message. There might be a more standard way to do that, but this looks like current code, using our Threads.createThread stuff. I don't know if there's a better way to do this.
   
   * I have no idea why we're trying to catch UnsatisfiedLinkError in the MinorCompactor. We probably shouldn't do that, as any checking of that should already be handled by our native code loader. It's possible this was added to address native loading of Hadoop native code, and it could leak out of Hadoop APIs, so we're trying to catch it here, but I have no idea.
   
   * There's a comment explaining why NoClassDefFoundError is being used to handle transient user errors in MinorCompactor that are due to temporary misconfigurations. I'm not sure under what circumstances this could be reached, though. If a user is using a context with a separate classloader for that context, I would expect user errors like this to be handled in the context's classloader. I'm not sure what the default behavior is if the user has not specified a context, or if the context is configured to user the normal system classloader. This one needs some further investigation/experimentation to see how it interoperates with the user threads and contexts.
   
   * Tablet catching Error is weird. It's specifically catching `Exception | Error`, when that's just `Throwable`. (Aside: other than tests, which are fine, we also catch Throwable in TabletServerBatchReaderIterator specifically to detect the fatal problem for the purposes of customizing a logged DEBUG message and rethrowing; that's probably okay, but as far as I can tell, that's the only other occurrence of what is effectively catching Throwable, so I thought it worth a mention here.) Here, Tablet is catching this in case an Error was thrown during the minor compaction. We're already trying to handle other Errors (see above) for minor compactions. I'm not sure what added value this one is serving.
   


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org