Sorry, I didn’t make clear which suggestion I was responding to, as there are multiple suggestions in the thread.
Adding new exceptions is fine, as long as it doesn’t break old clients like Thejas mentions. And clearly cleaning up NPEs is good. Changing which exceptions are thrown when a particular error is hit (e.g. throwing InvalidObjectException when a table is created in a non-existent database rather than the more reasonable NoSuchObject exception) I am less sure of, as users have probably adapted to the existing behavior. My earlier response was to the larger suggestions that would remove or change methods, etc. I agree we need to be thinking about a cleaner, leaner, and more rational V2 of the API. We could then put new functionality in V2 and maintain the existing one for backwards compatibility. The first question I’d ask on V2 is, should we stick with Thrift? What if we switched to REST? Or Google RPC? Or something else. I’m not saying we should switch, but it’s worth thinking through. (Switching would come with a high cost, since we pass all the objects around as thrift objects internally, so it may not be worth it. But that’s discussion we can have when we start designing API V2…) Alan. On Fri, Dec 15, 2017 at 10:42 AM, Thejas Nair <thejas.n...@gmail.com> wrote: > 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 > > > >> > > > >> > > > > > > > > >
