Accumulo
  1. Accumulo
  2. ACCUMULO-1965

Invalid table names (& namespaces) should have dedicated error codes

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6.0
    • Component/s: client
    • Labels:
      None

      Description

      To improve the client API, we should minimize the number of exceptions that require String parsing to determine the exception type. Table naming errors is one of them.

        Issue Links

          Activity

          Hide
          Christopher Tubbs added a comment -

          Following the guidelines in "Effective Java", this type of error seems to be of the "programmer error" type, and as such, I think a RuntimeException with an informative error message is perfectly acceptable (so long as the message gets propagated to the client).

          Show
          Christopher Tubbs added a comment - Following the guidelines in "Effective Java", this type of error seems to be of the "programmer error" type, and as such, I think a RuntimeException with an informative error message is perfectly acceptable (so long as the message gets propagated to the client).
          Hide
          John Vines added a comment -

          While that is great for user facing applications, I do not interpret Accumulo as that. Accumulo is a backend, and it is more likely for apps to be written around the accumulo client API. Unless you want to force the checking onto programmer writing a client, at which point you need to make the method for validating table names part of the public API. Either method is acceptable to me, but the former seemed the more straight forward and useful.

          Show
          John Vines added a comment - While that is great for user facing applications, I do not interpret Accumulo as that. Accumulo is a backend, and it is more likely for apps to be written around the accumulo client API. Unless you want to force the checking onto programmer writing a client, at which point you need to make the method for validating table names part of the public API. Either method is acceptable to me, but the former seemed the more straight forward and useful.
          Hide
          Josh Elser added a comment -

          Either method is acceptable to me, but the former seemed the more straight forward and useful.

          Agreed

          Show
          Josh Elser added a comment - Either method is acceptable to me, but the former seemed the more straight forward and useful. Agreed
          Hide
          Christopher Tubbs added a comment -

          I think API design principles tend to apply regardless of the argument of whether or not the API is "user facing". I can't see how we could even make that distinction, anyway... because all APIs are subject to being wrapped.

          I just don't see a problem with the basic IllegalArgumentException with an informative message. If the API is wrapped and gets arguments from users, it should do its own argument validation before passing to Accumulo's API. We could make it easier to do this argument validation, though... and I think I'd prefer that option if there's a sensible way to do that. However, we already throw IllegalArgumentException for a lot of our API if the argument is unexpectedly null... and from an API perspective, I'm not sure this kind of error is functionally any different than that kind of error... they're both illegal arguments.

          In essence, I think: if we document what are valid arguments, and you're a consumer of that API, then you need to ensure that the arguments you provide are valid arguments. This is true for anything using the API... whether you are an end user, or a wrapper.

          Show
          Christopher Tubbs added a comment - I think API design principles tend to apply regardless of the argument of whether or not the API is "user facing". I can't see how we could even make that distinction, anyway... because all APIs are subject to being wrapped. I just don't see a problem with the basic IllegalArgumentException with an informative message. If the API is wrapped and gets arguments from users, it should do its own argument validation before passing to Accumulo's API. We could make it easier to do this argument validation, though... and I think I'd prefer that option if there's a sensible way to do that. However, we already throw IllegalArgumentException for a lot of our API if the argument is unexpectedly null... and from an API perspective, I'm not sure this kind of error is functionally any different than that kind of error... they're both illegal arguments. In essence, I think: if we document what are valid arguments, and you're a consumer of that API, then you need to ensure that the arguments you provide are valid arguments. This is true for anything using the API... whether you are an end user, or a wrapper.
          Hide
          Sean Busbey added a comment -

          In essence, I think: if we document what are valid arguments, and you're a consumer of that API, then you need to ensure that the arguments you provide are valid arguments. This is true for anything using the API... whether you are an end user, or a wrapper.

          +1 I agree with Christopher. Consider some example uses of our API.

          with additional exception types:

          public void userProvidedTable(String table) {
            try {
              connection.tableOperations().create(table);
              // ... 
            } catch (InvalidTableNameException exception) {
              // Provide user feedback about problem
            }
          }
          

          with an validity helper method

          public void userProvidedTable(String table) {
            if (!(TableOperationsHelper.isValidTableName(table))) {
              // Provide user feedback about problem
            }
            connection.tableOperations().create(table);
            // ...
          }
          

          The validity of the table name isn't an unexpected situation that you can't know before you make the call (like e.g. if the table will exist when Accumulo goes to actually make the table).

          Show
          Sean Busbey added a comment - In essence, I think: if we document what are valid arguments, and you're a consumer of that API, then you need to ensure that the arguments you provide are valid arguments. This is true for anything using the API... whether you are an end user, or a wrapper. +1 I agree with Christopher. Consider some example uses of our API. with additional exception types: public void userProvidedTable( String table) { try { connection.tableOperations().create(table); // ... } catch (InvalidTableNameException exception) { // Provide user feedback about problem } } with an validity helper method public void userProvidedTable( String table) { if (!(TableOperationsHelper.isValidTableName(table))) { // Provide user feedback about problem } connection.tableOperations().create(table); // ... } The validity of the table name isn't an unexpected situation that you can't know before you make the call (like e.g. if the table will exist when Accumulo goes to actually make the table).
          Hide
          John Vines added a comment -

          Valid table names aren't quite the same as null arguments. We should provide a utility for checking because users shouldn't depend on constants.valid table regex.

          Then, if they still fail, it could be an illegal argument exception or a typed exception (but never just a runtime exception) . And an illegal argument exception should be fine because that's the only real argument to create table.

          So either solution should work, but it seems like we should have a table name check utility available to users in the client api.

          Show
          John Vines added a comment - Valid table names aren't quite the same as null arguments. We should provide a utility for checking because users shouldn't depend on constants.valid table regex. Then, if they still fail, it could be an illegal argument exception or a typed exception (but never just a runtime exception) . And an illegal argument exception should be fine because that's the only real argument to create table. So either solution should work, but it seems like we should have a table name check utility available to users in the client api.
          Hide
          Sean Busbey added a comment -

          Then, if they still fail, it could be an illegal argument exception or a typed exception (but never just a runtime exception) . And an illegal argument exception should be fine because that's the only real argument to create table.

          I agree that it should be an IllegalArgumentException.

          but it seems like we should have a table name check utility available to users in the client api.

          AFAICT, we all agree on this point. How about going with a static method on TableOperationsHelper?

          As an aside, do we have table name restrictions documented somewhere? Other than Constants.VALID_TABLE_REGEX, I don't see anything and that's not in the public API. Namespaces have a note in the javadocs for the create method on NamespaceOperations.

          Show
          Sean Busbey added a comment - Then, if they still fail, it could be an illegal argument exception or a typed exception (but never just a runtime exception) . And an illegal argument exception should be fine because that's the only real argument to create table. I agree that it should be an IllegalArgumentException. but it seems like we should have a table name check utility available to users in the client api. AFAICT, we all agree on this point. How about going with a static method on TableOperationsHelper? As an aside, do we have table name restrictions documented somewhere? Other than Constants.VALID_TABLE_REGEX, I don't see anything and that's not in the public API. Namespaces have a note in the javadocs for the create method on NamespaceOperations.
          Hide
          Keith Turner added a comment -

          As an aside, do we have table name restrictions documented somewhere?

          This would be nice to have in the javadoc

          Show
          Keith Turner added a comment - As an aside, do we have table name restrictions documented somewhere? This would be nice to have in the javadoc
          Hide
          John Vines added a comment -

          Actually, looked at the code. Invalid table name presents as an AccumuloException, where as a null table name or null TimeType present as IllegalArgumentException. This is a good start, because having ambiguity between not providing a required field vs. having an invalid field type is an issue. My main gripe currently is that non-existence of a namespace presents as the exact same error as an invalid table name (as well as conflict with meta table names), which results in needing to parse error messages in order to disambiguate. There is a similar issue, though less critical, with namespace creation.

          Show
          John Vines added a comment - Actually, looked at the code. Invalid table name presents as an AccumuloException, where as a null table name or null TimeType present as IllegalArgumentException. This is a good start, because having ambiguity between not providing a required field vs. having an invalid field type is an issue. My main gripe currently is that non-existence of a namespace presents as the exact same error as an invalid table name (as well as conflict with meta table names), which results in needing to parse error messages in order to disambiguate. There is a similar issue, though less critical, with namespace creation.
          Hide
          Sean Busbey added a comment -

          My main gripe currently is that non-existence of a namespace presents as the exact same error as an invalid table name

          Shouldn't this use NamespaceNotFoundException ?

          Show
          Sean Busbey added a comment - My main gripe currently is that non-existence of a namespace presents as the exact same error as an invalid table name Shouldn't this use NamespaceNotFoundException ?
          Hide
          John Vines added a comment -

          It should, but then it's an API change. Currently that exception is only used by clone and importTable, in which they are inconsistently wrapped in IllegalArgumentExceptions and RuntimeExceptions, respectively.

          Show
          John Vines added a comment - It should, but then it's an API change. Currently that exception is only used by clone and importTable, in which they are inconsistently wrapped in IllegalArgumentExceptions and RuntimeExceptions, respectively.
          Hide
          John Vines added a comment -

          That was why I suggested that NamespaceNotFound/Exists/NotEmpty should all extend AccumuloException to get around those issues (and ultimately I think the various Table* exceptions should as well), but that idea was rejected without consensus here https://reviews.apache.org/r/15166/#comment54506

          Show
          John Vines added a comment - That was why I suggested that NamespaceNotFound/Exists/NotEmpty should all extend AccumuloException to get around those issues (and ultimately I think the various Table* exceptions should as well), but that idea was rejected without consensus here https://reviews.apache.org/r/15166/#comment54506
          Hide
          Sean Busbey added a comment -

          It should, but then it's an API change.

          This is all going in 1.6.0, right? I thought we made API changes across major versions?

          That was why I suggested that NamespaceNotFound/Exists/NotEmpty should all extend AccumuloException to get around those issues

          Code written against the create table API in 1.5 couldn't have known about namespaces. If we're making API change decisions with an eye towards making said code work in a mixed environment, extending AccumuloException would be a reasonable way to do that.

          The reviewboard feedback you mention looks mostly ambivalent. Maybe this particular set of implications is worth its own ticket+patch or a mailing list thread?

          Show
          Sean Busbey added a comment - It should, but then it's an API change. This is all going in 1.6.0, right? I thought we made API changes across major versions? That was why I suggested that NamespaceNotFound/Exists/NotEmpty should all extend AccumuloException to get around those issues Code written against the create table API in 1.5 couldn't have known about namespaces. If we're making API change decisions with an eye towards making said code work in a mixed environment, extending AccumuloException would be a reasonable way to do that. The reviewboard feedback you mention looks mostly ambivalent. Maybe this particular set of implications is worth its own ticket+patch or a mailing list thread?
          Hide
          John Vines added a comment -

          Done, ACCUMULO-1970

          As for API changes, we can add APIs no problem, but changing existing ones require a deprecation cycle. This includes adding new exception types to the signature.

          Show
          John Vines added a comment - Done, ACCUMULO-1970 As for API changes, we can add APIs no problem, but changing existing ones require a deprecation cycle. This includes adding new exception types to the signature.
          Hide
          Christopher Tubbs added a comment -

          Okay, I'm getting a grasp on what the bug actually is and what needs to be fixed.

          Table operations like clone table and create table currently throw a TableNotFoundException, wrapped in a RuntimeException (because they're not technically supposed to happen (we should be throwing an AssertionError here, not a RuntimeException, anyway), but do, because we're re-using the ThriftTableOperationException to return a NOTFOUND code, which is interpreted as a TableNotFoundException).

          I will fix this.

          I could not find the importTable problem John Vines was referring to.

          I don't think this problem extends beyond clone and create, because in all other cases, we're only looking for an existing fully-qualified table, and it doesn't really matter why the table doesn't exist. In fact, it shouldn't even have to reveal more information than necessary (like the fact that a namespace exists or doesn't exist) when the fully-qualified table doesn't exist.

          Show
          Christopher Tubbs added a comment - Okay, I'm getting a grasp on what the bug actually is and what needs to be fixed. Table operations like clone table and create table currently throw a TableNotFoundException, wrapped in a RuntimeException (because they're not technically supposed to happen (we should be throwing an AssertionError here, not a RuntimeException, anyway), but do, because we're re-using the ThriftTableOperationException to return a NOTFOUND code, which is interpreted as a TableNotFoundException). I will fix this. I could not find the importTable problem John Vines was referring to. I don't think this problem extends beyond clone and create, because in all other cases, we're only looking for an existing fully-qualified table, and it doesn't really matter why the table doesn't exist. In fact, it shouldn't even have to reveal more information than necessary (like the fact that a namespace exists or doesn't exist) when the fully-qualified table doesn't exist.
          Hide
          Christopher Tubbs added a comment -

          Oops, didn't mean to add previous comment... it's old, before further discussions with John Vines. What we've settled on is sending back an opcode for the namespace not found over the wire, and on the client side turning the opcode into an AccumuloException containing a NamespaceNotFoundException.

          The intent is to revisit this in the future and see what we can do to clean this up.

          Show
          Christopher Tubbs added a comment - Oops, didn't mean to add previous comment... it's old, before further discussions with John Vines . What we've settled on is sending back an opcode for the namespace not found over the wire, and on the client side turning the opcode into an AccumuloException containing a NamespaceNotFoundException. The intent is to revisit this in the future and see what we can do to clean this up.
          Hide
          ASF subversion and git services added a comment -

          Commit f35e3f47265868c560b3d49edebf1a8c24196512 in branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f35e3f4 ]

          ACCUMULO-1965 Fix exception handling for namespaces

          Make the exception handling propagate correctly to the client.
          Vastly expanded on existing namespace tests to include checks for
          throwing the correct exceptions. Consolidated a lot of the fate
          operations code and refactored the master (slightly; moved inner
          classes to separate files) to be easier to modify the relevant RPC
          handling code.

          Fixed many API bugs related to throwing correct exceptions, found from
          the new tests added to NamespacesIT.

          Show
          ASF subversion and git services added a comment - Commit f35e3f47265868c560b3d49edebf1a8c24196512 in branch refs/heads/1.6.0-SNAPSHOT from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f35e3f4 ] ACCUMULO-1965 Fix exception handling for namespaces Make the exception handling propagate correctly to the client. Vastly expanded on existing namespace tests to include checks for throwing the correct exceptions. Consolidated a lot of the fate operations code and refactored the master (slightly; moved inner classes to separate files) to be easier to modify the relevant RPC handling code. Fixed many API bugs related to throwing correct exceptions, found from the new tests added to NamespacesIT.
          Hide
          ASF subversion and git services added a comment -

          Commit f35e3f47265868c560b3d49edebf1a8c24196512 in branch refs/heads/master from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f35e3f4 ]

          ACCUMULO-1965 Fix exception handling for namespaces

          Make the exception handling propagate correctly to the client.
          Vastly expanded on existing namespace tests to include checks for
          throwing the correct exceptions. Consolidated a lot of the fate
          operations code and refactored the master (slightly; moved inner
          classes to separate files) to be easier to modify the relevant RPC
          handling code.

          Fixed many API bugs related to throwing correct exceptions, found from
          the new tests added to NamespacesIT.

          Show
          ASF subversion and git services added a comment - Commit f35e3f47265868c560b3d49edebf1a8c24196512 in branch refs/heads/master from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f35e3f4 ] ACCUMULO-1965 Fix exception handling for namespaces Make the exception handling propagate correctly to the client. Vastly expanded on existing namespace tests to include checks for throwing the correct exceptions. Consolidated a lot of the fate operations code and refactored the master (slightly; moved inner classes to separate files) to be easier to modify the relevant RPC handling code. Fixed many API bugs related to throwing correct exceptions, found from the new tests added to NamespacesIT.

            People

            • Assignee:
              Christopher Tubbs
              Reporter:
              John Vines
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development