It makes perfect sense to me to clean up the API when we release the standalone MetaStore. +1 for the API cleanup in general!
> On Dec 15, 2017, at 1:53 AM, Alexander Kolbasov <[email protected]> wrote: > > +1 for API cleanup in general! > > On Thu, Dec 14, 2017 at 4:25 PM, Sergey Shelukhin <[email protected]> > wrote: > >> If we break the APIs, can we also do the API cleanup where we remove >> duplicate ones and change everything to use req/resp pattern? >> >> On 17/12/14, 13:50, "Thejas Nair" <[email protected]> wrote: >> >>> This direction looks good to me. >>> If the new exceptions are inheriting from TException the applications >>> would >>> still work. But would it still work if we old metastore client library is >>> used with a newer version of metastore server running with these changes ? >>> >>> >>> On Wed, Dec 13, 2017 at 4:02 AM, 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 >>>> >>>> >> >>
