You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary <pv...@cloudera.com> on 2017/12/13 12:02:10 UTC
[DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
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
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Peter Vary <pv...@cloudera.com>.
Hi Alan,
I think I understand your point. If we come up with the standalone MetaStore but the interface is completely different then the original one, then we will lose in adoption. I agree with your assessment.
Do you (or anyone) have any ideas, how can we rationalize the interface without disrupting the use cases you mentioned?
Sooner or later maintaining those 37 different ways to fetch partitions will be a huge burden, especially if we start adding new features like HIVE-17990, and hopefully others that come after that.
For example in Hadoop and Spark I see that there are "deprecated" methods which are getting removed after major releases. Do we have plans for this?
There are some boundaries which I did not feel yet, so I would like to know how the more experienced Hive developers feel about what constitutes a breaking change?
Is it a breaking change, if for a specific error the API was throwing InvalidObjectException, but after the change it will throw NoSuchObjectException -even if the method declaration does not need to be change for this?
Is it acceptable if we add a configuration value to the MetaStore to disable/enable throwing new exceptions? Like if "compatibility mode" is true, then the old exceptions are thrown, if it is false, then the new exceptions are thrown.
Note, that the complexity of the 2. version is only worth it, if we plan to adapt something like the Hadoop model, and sooner or later deprecate the old versions.
Thanks for joining the conversation, and highlighting important constraints!
Peter
> On Dec 15, 2017, at 4:33 PM, Alan Gates <al...@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 <th...@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
>>>>
>>>>
>>
>>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alan Gates <al...@gmail.com>.
+1.
Alan.
On Tue, Dec 19, 2017 at 8:14 AM, Peter Vary <pv...@cloudera.com> 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 <al...@gmail.com> 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 <th...@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 <al...@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 <th...@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
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>
> >>
>
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Peter Vary <pv...@cloudera.com>.
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 <al...@gmail.com> 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 <th...@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 <al...@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 <th...@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
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alan Gates <al...@gmail.com>.
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 <th...@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 <al...@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 <th...@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
> > > >>
> > > >>
> > >
> > >
> >
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Thejas Nair <th...@gmail.com>.
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 <al...@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 <th...@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
> > >>
> > >>
> >
> >
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alan Gates <al...@gmail.com>.
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 <th...@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
> >>
> >>
>
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Peter Vary <pv...@cloudera.com>.
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 <th...@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
>>
>>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Peter Vary <pv...@cloudera.com>.
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 <ak...@cloudera.com> wrote:
>
> +1 for API cleanup in general!
>
> On Thu, Dec 14, 2017 at 4:25 PM, Sergey Shelukhin <se...@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" <th...@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
>>>>
>>>>
>>
>>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alexander Kolbasov <ak...@cloudera.com>.
+1 for API cleanup in general!
On Thu, Dec 14, 2017 at 4:25 PM, Sergey Shelukhin <se...@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" <th...@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
> >>
> >>
>
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Sergey Shelukhin <se...@hortonworks.com>.
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" <th...@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
>>
>>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alexander Kolbasov <ak...@cloudera.com>.
What are expectations for Hive 3 in terms of API compatibility - especially
in regards to old clients talking to Hive 3 servers?
- Alex
On Thu, Dec 14, 2017 at 1:50 PM, Thejas Nair <th...@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
> >
> >
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Thejas Nair <th...@gmail.com>.
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
>
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Alexander Kolbasov <ak...@cloudera.com>.
What is the "official status" of these APIs? Were they ever documented and
advertised as public? Or clients were always using them at their own risk?
Were there any commitment to maintain compatibility of these APIs?
Which is considered "more official" - the Thrift APIs or IMetastore ones?
Which ones should be used by 3-rd party applications (or should any of
these be used at all?)
- Alex
On Thu, Dec 14, 2017 at 4:56 AM, Peter Vary <pv...@cloudera.com> wrote:
> Hi,
>
> Is it acceptable to introduce backward incompatible changes in the Thrift
> interface in the 3.0.0. release?
> If we want to handle the validation on the server side - so every Thrift
> user can benefit from the changes - then we have to add new exceptions to
> the throws clause on the method declared on the Thrift interface as well.
> For example:
> --- a/standalone-metastore/src/main/thrift/hive_metastore.thrift
> +++ b/standalone-metastore/src/main/thrift/hive_metastore.thrift
> @@ -1653,15 +1653,16 @@ service ThriftHiveMetastore extends
> fb303.FacebookService
> 4:NoSuchObjectException o4)
>
> void drop_function(1:string dbName, 2:string funcName)
> - throws (1:NoSuchObjectException o1, 2:MetaException o3)
> + throws (1:NoSuchObjectException o1, 2:InvalidObjectException o2,
> 3:MetaException o3)
>
> void alter_function(1:string dbName, 2:string funcName, 3:Function
> newFunc)
> - throws (1:InvalidOperationException o1, 2:MetaException o2)
> + throws (1:AlreadyExistsException o1, 2:InvalidObjectException o2,
> 3:NoSuchObjectException o3,
> + 4:MetaException o4)
>
> list<string> get_functions(1:string dbName, 2:string pattern)
> - throws (1:MetaException o1)
> + throws (1:MetaException o1, 2:InvalidObjectException o2)
> Function get_function(1:string dbName, 2:string funcName)
> - throws (1:MetaException o1, 2:NoSuchObjectException o2)
> + throws (1:MetaException o1, 2:NoSuchObjectException o2,
> 3:InvalidObjectException o3)
>
> Thanks,
> Peter
>
> > On Dec 13, 2017, at 1:02 PM, 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
> >
>
>
Re: [DISCUSS] IMetaStoreClient and HMS Thrift API exception handling
Posted by Peter Vary <pv...@cloudera.com>.
Hi,
Is it acceptable to introduce backward incompatible changes in the Thrift interface in the 3.0.0. release?
If we want to handle the validation on the server side - so every Thrift user can benefit from the changes - then we have to add new exceptions to the throws clause on the method declared on the Thrift interface as well.
For example:
--- a/standalone-metastore/src/main/thrift/hive_metastore.thrift
+++ b/standalone-metastore/src/main/thrift/hive_metastore.thrift
@@ -1653,15 +1653,16 @@ service ThriftHiveMetastore extends fb303.FacebookService
4:NoSuchObjectException o4)
void drop_function(1:string dbName, 2:string funcName)
- throws (1:NoSuchObjectException o1, 2:MetaException o3)
+ throws (1:NoSuchObjectException o1, 2:InvalidObjectException o2, 3:MetaException o3)
void alter_function(1:string dbName, 2:string funcName, 3:Function newFunc)
- throws (1:InvalidOperationException o1, 2:MetaException o2)
+ throws (1:AlreadyExistsException o1, 2:InvalidObjectException o2, 3:NoSuchObjectException o3,
+ 4:MetaException o4)
list<string> get_functions(1:string dbName, 2:string pattern)
- throws (1:MetaException o1)
+ throws (1:MetaException o1, 2:InvalidObjectException o2)
Function get_function(1:string dbName, 2:string funcName)
- throws (1:MetaException o1, 2:NoSuchObjectException o2)
+ throws (1:MetaException o1, 2:NoSuchObjectException o2, 3:InvalidObjectException o3)
Thanks,
Peter
> On Dec 13, 2017, at 1:02 PM, 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
>