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" <pxi...@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
