Alan, The changes suggested by Peter was to add another checked exception, which is a subclass of TException. And TException is already being thrown by all thrift api calls. So it should not break any applications. The only concern I have is some issues if old client is used with new servers. We need to verifiy that the old client is able to deserialize a response with new exceptions.
On Fri, Dec 15, 2017 at 7:33 AM, Alan Gates <alanfga...@gmail.com> wrote: > We do not want to break either the Thrift APIs or the IMetaStoreClient > ones. I do not know if we have ever declared them to be public or not, but > they are de facto public in that everyone uses them. Consider that these > are used by Impala, Presto, Qubole, Amazon's Glue, Spark, and probably > others we don't know about. If we produce a new version that does not > maintain those APIs everyone will just ignore it and stay on the older > version (just like python 3). I would even include changing exception > types in this. > > I would like to have a clean API with consistent error handling and that > does not include 37 different ways to fetch partitions (I exaggerate only > slightly), but we need to find a way to do it that does not force users to > rewrite their application to upgrade to the standalone metastore. > > Alan. > > > On Fri, Dec 15, 2017 at 2:20 AM, Peter Vary <pv...@cloudera.com> wrote: > > > If the application using the old API does not handle the original > > exceptions differently than the TExceptions then it should work as > expected. > > > > > On Dec 14, 2017, at 10:50 PM, 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 > > >> > > >> > > > > >
