You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Pengcheng Xiong <px...@apache.org> on 2016/01/26 01:15:32 UTC
Need your opinion on HIVE-12916 to clean up metastore API
Hi all,
I was working on a new feature in HIVE-12730 and I was suggested by
Ashutosh to put a configuration in EnvironmentContext. Then I found that we
have lots of cases where the configuration and EnvironmentContext both
exist in separate functions in the API list. For example, we have
public void alter_table(String dbname, String tbl_name, Table new_tbl)
throws InvalidOperationException, MetaException,
org.apache.thrift.TException;
public void alter_table_with_environment_context(String dbname, String
tbl_name, Table new_tbl, EnvironmentContext environment_context) throws
InvalidOperationException, MetaException, org.apache.thrift.TException;
public void alter_table_with_cascade(String dbname, String tbl_name, Table
new_tbl, boolean cascade) throws InvalidOperationException, MetaException,
org.apache.thrift.TException;
My plan is to keep only
public void alter_table_with_environment_context(String dbname, String
tbl_name, Table new_tbl, EnvironmentContext environment_context)
throws InvalidOperationException,
MetaException, org.apache.thrift.TException;
And then merge the other two into this single one at both client and
server side. For example, put cascade in the environment_context at client
side and then read it from server side. If we do so, then it is easy to
extend in the future, for example, my current work in HIVE-12730.
The patch is already there but Sergey has some concerns about backward
compatibility. Could you please leave your comments/suggestions? Thanks a
lot.
Best
Pengcheng
Re: Need your opinion on HIVE-12916 to clean up metastore API
Posted by Alan Gates <al...@gmail.com>.
Are you proposing to do this in 2.0 or branch-1 or both? In master we
can put up with some backwards incompatibility for now. But given that
it's straightforward to maintain it (as Sergey outlines) even there it
doesn't seem worth it. So unless there's a very compelling reason (i.e.
more than just code/API cleanup) we should maintain backwards
compatibility. And we have to maintain it in branch-1.
Alan.
> Sergey Shelukhin <ma...@hortonworks.com>
> January 25, 2016 at 17:30
> There are 2 separate issues.
>
> 1) Removing old APIs. There’s (almost) no impact from keeping the old
> function for compat if both old and new call the same internal method with
> slight argument variations, but there would be compact problems with
> removing them if someone is using it; metastore is used by many projects
> across the ecosystem, and I don’t know what functions they might be using.
> So I think we should keep the function. So far I’m -1 on removing (or
> breaking signature changes).
>
> 1a) We can deprecate the functions (in release notes, I presume; Thrift
> doesn’t support deprecation as far as I know), then remove them in next
> release, but I think we should do it in a systematic manner with entire
> metastore API, if there’s will to do the work.
>
> 2) Adding new arguments to old APIs.
> I’d rather that we add new API based on Request/Response pattern for such
> cases, those are easy to evolve. add_partitions vs add_partitions_req is
> one example.
> Then, as part of 1a, we can also remove the old APIs.
> I am -0 on that part of the JIRA, I guess it should be ok to (ab)use
> EnvironmentContext for additional parameters ;) As long as the change is
> backward compatible.
>
>
>
>
>
> Pengcheng Xiong <ma...@apache.org>
> January 25, 2016 at 16:15
> Hi all,
>
> I was working on a new feature in HIVE-12730 and I was suggested by
> Ashutosh to put a configuration in EnvironmentContext. Then I found
> that we
> have lots of cases where the configuration and EnvironmentContext both
> exist in separate functions in the API list. For example, we have
>
> public void alter_table(String dbname, String tbl_name, Table new_tbl)
> throws InvalidOperationException, MetaException,
> org.apache.thrift.TException;
>
> public void alter_table_with_environment_context(String dbname, String
> tbl_name, Table new_tbl, EnvironmentContext environment_context) throws
> InvalidOperationException, MetaException, org.apache.thrift.TException;
>
> public void alter_table_with_cascade(String dbname, String tbl_name, Table
> new_tbl, boolean cascade) throws InvalidOperationException, MetaException,
> org.apache.thrift.TException;
>
> My plan is to keep only
>
> public void alter_table_with_environment_context(String dbname, String
> tbl_name, Table new_tbl, EnvironmentContext environment_context)
> throws InvalidOperationException,
> MetaException, org.apache.thrift.TException;
>
> And then merge the other two into this single one at both client and
> server side. For example, put cascade in the environment_context at client
> side and then read it from server side. If we do so, then it is easy to
> extend in the future, for example, my current work in HIVE-12730.
>
> The patch is already there but Sergey has some concerns about backward
> compatibility. Could you please leave your comments/suggestions? Thanks a
> lot.
>
>
> Best
>
> Pengcheng
>
Re: Need your opinion on HIVE-12916 to clean up metastore API
Posted by Sergey Shelukhin <se...@hortonworks.com>.
There are 2 separate issues.
1) Removing old APIs. There’s (almost) no impact from keeping the old
function for compat if both old and new call the same internal method with
slight argument variations, but there would be compact problems with
removing them if someone is using it; metastore is used by many projects
across the ecosystem, and I don’t know what functions they might be using.
So I think we should keep the function. So far I’m -1 on removing (or
breaking signature changes).
1a) We can deprecate the functions (in release notes, I presume; Thrift
doesn’t support deprecation as far as I know), then remove them in next
release, but I think we should do it in a systematic manner with entire
metastore API, if there’s will to do the work.
2) Adding new arguments to old APIs.
I’d rather that we add new API based on Request/Response pattern for such
cases, those are easy to evolve. add_partitions vs add_partitions_req is
one example.
Then, as part of 1a, we can also remove the old APIs.
I am -0 on that part of the JIRA, I guess it should be ok to (ab)use
EnvironmentContext for additional parameters ;) As long as the change is
backward compatible.
On 16/1/25, 16:15, "Pengcheng Xiong" <px...@apache.org> wrote:
>Hi all,
>
> I was working on a new feature in HIVE-12730 and I was suggested by
>Ashutosh to put a configuration in EnvironmentContext. Then I found that
>we
>have lots of cases where the configuration and EnvironmentContext both
>exist in separate functions in the API list. For example, we have
>
>public void alter_table(String dbname, String tbl_name, Table new_tbl)
>throws InvalidOperationException, MetaException,
>org.apache.thrift.TException;
>
>public void alter_table_with_environment_context(String dbname, String
>tbl_name, Table new_tbl, EnvironmentContext environment_context) throws
>InvalidOperationException, MetaException, org.apache.thrift.TException;
>
>public void alter_table_with_cascade(String dbname, String tbl_name, Table
>new_tbl, boolean cascade) throws InvalidOperationException, MetaException,
>org.apache.thrift.TException;
>
> My plan is to keep only
>
>public void alter_table_with_environment_context(String dbname, String
>tbl_name, Table new_tbl, EnvironmentContext environment_context)
>throws InvalidOperationException,
>MetaException, org.apache.thrift.TException;
>
> And then merge the other two into this single one at both client and
>server side. For example, put cascade in the environment_context at client
>side and then read it from server side. If we do so, then it is easy to
>extend in the future, for example, my current work in HIVE-12730.
>
> The patch is already there but Sergey has some concerns about backward
>compatibility. Could you please leave your comments/suggestions? Thanks a
>lot.
>
>
>Best
>
>Pengcheng