On Tue, Jun 10, 2025 at 7:52 PM Dave Marion <dmario...@gmail.com> wrote: > > I made a comment regarding the TableNotFoundException hierarchy at [0]. I > don't understand why our specific exceptions (TNFE for example) don't > extend AccumuloException. If the hierarchy is changed in our Exception
Mainly because it's a breaking change. > classes, then the signatures of the other methods don't need to be changed. > Calling code that catches AccumuloException will still work. Calling code It won't necessarily still work... at least, not necessarily the way the user intends. This change would make the catch order-dependent, and may cause compilation errors due to unreachable code blocks, if the AccumuloException is caught first. > that catches AccumuloException and checks the cause for TNFE won't work > unless we do something like call `super.initCause(this)` after initializing > the parent Exception. > > [0] https://github.com/apache/accumulo/pull/5595#issuecomment-2922377649 If we could go back in time and make a common base class, then that would be ideal. Instead, we have to build on the history that exists. One way we can do this is by creating a new set of exceptions with proper hierarchies, in a o.a.a.core.ex.* package. We can then make these new exceptions the parents of the existing ones, which can be deprecated and removed in a future major release. This, of course, leaves the definition of "proper hierarchies" to be determined, and we would want to be careful not to leak non-public types into these new classes (some Exceptions already reference non-public types, like thrift types, so they can be constructed at particular places in the code. We may want to avoid making those constructors available, by converting any non-public types prior to construction of the Exceptions, so as to avoid these kinds of API leaks, but that could get tricky in some places.) > > On Tue, Jun 10, 2025 at 3:53 PM Dominic Garguilo <domgargu...@apache.org> > wrote: > > > Hi all, > > > > I'd like to discuss how TableNotFoundException (TNFE) is handled in > > some of our TableOperations methods. In several places, TNFE is > > neither declared nor thrown; instead it’s wrapped in an > > AccumuloException (AE). That pushes the burden onto calling code to > > discover the wrapping (by reading Javadoc or source) and then to > > handle it—both error-prone. > > > > Obviously, the "right" solution is to have all applicable methods > > declare and throw TNFE directly. The blocker is that it’s a public-API > > change. This came up in the context of Accumulo PR #5540 [1], which > > refactors some TableOperationsImpl methods to use a single > > modifyProperties() call instead of multiple > > setProperty()/removeProperty() calls for atomicity. Since > > modifyProperties() doesn’t throw TNFE (but wraps it), I implemented a > > hacky workaround to unwrap and re-throw TNFE without changing the API. > > We should replace that workaround with a more permanent solution, so > > I’m opening this thread to discuss possible paths forward. > > > > Possible paths forward: > > > > 1. Backport the workaround into 2.1 > > Continue using the internal "unwrap AE -> TNFE" helper. Preserves > > the API and atomic benefits of modifyProperties(), but perpetuates the > > hacky code. > > > > 2. Break SemVer in 2.1 > > Change existing signatures so that modifyProperties(), > > setProperty(), etc., declare and throw TNFE directly. Clean and > > immediate, but violates SemVer and could break subclasses. > > > > 3. Deprecate & add replacements in 2.1 > > Mark the old methods @Deprecated and introduce new ones (e.g. > > mutateProperties()) with correct throws. Signals intent clearly, but > > adds API churn in a minor release. > > > > 4. Postpone public-API fixes until 3.1/4.0 > > - In 2.1: use the unwrap helper internally where needed. > > - In 3.1: deprecate the old methods and introduce properly-declared > > replacements. > > - In 4.0: remove or adjust the deprecated methods entirely. > > > > Any and all suggestions or feedback are welcome! > > > > [1] https://github.com/apache/accumulo/pull/5540 > >
