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 <pv...@cloudera.com> 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 >