+1 for API cleanup in general! On Thu, Dec 14, 2017 at 4:25 PM, Sergey Shelukhin <ser...@hortonworks.com> 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" <thejas.n...@gmail.com> 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 <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 > >> > >> > >
