Hi,
Is it acceptable to introduce backward incompatible changes in the Thrift
interface in the 3.0.0. release?
If we want to handle the validation on the server side - so every Thrift user
can benefit from the changes - then we have to add new exceptions to the throws
clause on the method declared on the Thrift interface as well.
For example:
--- a/standalone-metastore/src/main/thrift/hive_metastore.thrift
+++ b/standalone-metastore/src/main/thrift/hive_metastore.thrift
@@ -1653,15 +1653,16 @@ service ThriftHiveMetastore extends
fb303.FacebookService
4:NoSuchObjectException o4)
void drop_function(1:string dbName, 2:string funcName)
- throws (1:NoSuchObjectException o1, 2:MetaException o3)
+ throws (1:NoSuchObjectException o1, 2:InvalidObjectException o2,
3:MetaException o3)
void alter_function(1:string dbName, 2:string funcName, 3:Function newFunc)
- throws (1:InvalidOperationException o1, 2:MetaException o2)
+ throws (1:AlreadyExistsException o1, 2:InvalidObjectException o2,
3:NoSuchObjectException o3,
+ 4:MetaException o4)
list<string> get_functions(1:string dbName, 2:string pattern)
- throws (1:MetaException o1)
+ throws (1:MetaException o1, 2:InvalidObjectException o2)
Function get_function(1:string dbName, 2:string funcName)
- throws (1:MetaException o1, 2:NoSuchObjectException o2)
+ throws (1:MetaException o1, 2:NoSuchObjectException o2,
3:InvalidObjectException o3)
Thanks,
Peter
> On Dec 13, 2017, at 1:02 PM, Peter Vary <[email protected]> wrote:
>
> Hi Team,
>
> Since the work begin to separate the HMS to a standalone project we thought
> that it would be useful the create extensive API tests for the public APIs of
> the new project.
> We started to create tests using the IMetaStoreClient interface
> implementations and found that not surprisingly the happy paths are working
> as expected, but there are some gaps in the exception handling. Most of the
> enhancements affect the Thrift API as well.
> We went through the following methods:
> Functions
> Indexes
> Tables
> Databases
> We also plan to comb through at least the Partition related methods too.
>
> The possible enhancements we found could be grouped to the following
> categories:
> Bugs: For example IMetaStoreClient.drop/alter/createFunction with null
> function name might throw NullPointerException exception - this is clearly a
> bug which should be solved:
> Embedded MetaStore throws NullPointerException
> Remote MetaStore client throws TTransportException
> Sub-optimal error handling: For example IMetaStoreClient.alterFunction will
> not check if the new function is already exist, and tries to insert it
> anyway. After 10 tries it throws a MetaException where the exception text is
> "Update of object [...] failed :
> java.sql.SQLIntegrityConstraintViolationException [...]". Fixing this could
> be done without interface change, but following the logic of the other
> methods on the interface AlreadyExistsException should be thrown.
> Inconsistent exception handling: Different methods will throw different
> exceptions for similar errors. This makes the interface hard to understand,
> hard to document and maintain. For example:
> Calling IMetaStoreClient.createTable with nonexistent database name will
> throw InvalidObjectException
> Calling IMetaStoreClient.createFunction with nonexistent database name
> database will throw NoSuchObjectException
> There are some cases when the Embedded MetaStore handles error differently
> than the Remote MetaStore. For example: IMetaStoreClient.dropTable with
> "null" as a database:
> Embedded MetaStore throws MetaException
> Remote MetaStore client throws TProtocolException
>
> Proposed changes:
>
> Fixing cases 1. and 2. is a simple bug fix - it could be done independently
> Fixing cases 3. and 4. will change how the IMetaStoreClient and HMS Thrift
> API works. For these we should review the IMetaStoreClient and HMS Thrift API
> interface exception handling, to create consistent and easy to follow rules
> for the possible exceptions. We propose to keep the current exceptions, and
> only change when the given type of exceptions are thrown. If we stick to this
> then the interface will be binary backward compatible since currently every
> method defines TException as a throwable and every exception is inherited
> from TException. I think we allowed to change this since 3.0.0 is a major
> release.
>
>
> Do we agree with the general direction of these changes?
>
> Thanks,
> Peter
>
