+1. Alan.
On Tue, Dec 19, 2017 at 8:14 AM, Peter Vary <[email protected]> wrote: > Hi, > > I did some testing. Added a new exception to the alter_function method on > server side and used an old client with erroneous date to get this new > exception. What I have found that a TApplicationException is thrown for the > user of the client, like this: > > org.apache.thrift.TApplicationException: alter_function failed: unknown > result > at org.apache.hadoop.hive.metastore.api. > ThriftHiveMetastore$Client.recv_alter_function( > ThriftHiveMetastore.java:4003) > at org.apache.hadoop.hive.metastore.api. > ThriftHiveMetastore$Client.alter_function(ThriftHiveMetastore.java:3988) > at org.apache.hadoop.hive.metastore.HiveMetaStoreClient. > alterFunction(HiveMetaStoreClient.java:2450) > at org.apache.hadoop.hive.metastore.client.TestFunctions. > testAlterFunctionInvalidData(TestFunctions.java:632) > > TApplicationException is a TException so theoretically the user can/should > handle this. > > After our discussion i had to reconsider and now I think, that if our main > goal is backward compatibility then we should not add new exceptions to the > API. Instead we should throw MetaException in this case. MetaException is > already often used with validation errors whereas TExceptions are mostly > infrastructure errors, so most probably handled differently. > > So the final solution would be for the current API: > Create tests > Fix errors > Fix suboptimal error handling on server side > Do not change method signature > If a new exception is needed use MetaException instead > > And on the long run, we should start to think about a API V2 with correct > versioning, and deprecation policies. > > Is it OK for everyone? > > Thanks, > Peter > > > > On Dec 15, 2017, at 8:15 PM, Alan Gates <[email protected]> wrote: > > > > 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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > 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 <[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 > >>>>>> > >>>>>> > >>>> > >>>> > >>> > >> > >
