[
https://issues.apache.org/jira/browse/HIVE-15062?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616849#comment-15616849
]
Sushanth Sowmyan edited comment on HIVE-15062 at 10/28/16 10:53 PM:
--------------------------------------------------------------------
I like the fundamental idea, and I think this makes sense. I would suggest a
couple of things, though.
a) checkClient is not really handling "checking" client compatibility. Instead,
it is instead acting as a gating assert for entering sections of code where we
make an assumption that feature exists. I think that an api style that did the
following would be more usable:
{code}
private boolean checkClientCompatible(ClientCapabilities capabilities,
ClientCapability value, String what, String call) {
// return true if compatible, false, if not. Simply test, no more.
...
}
private boolean assertClientCompatible(ClientCapabilities capabilities,
ClientCapability value, String what, String call) throws
MetaException {
if (!checkClientCompatible(capabilities, value, what, call) {
// throw exception similar to existing checkClient
}
}
{code}
Then, this allows us to assert that something should be compatible if it
requires it, and allows us to write backward compatible code if possible by
checking capability.
b) Also, I would advise cautionary use of the capabilities notion, since the
metastore api is a public api (also, the HCatClient api that sits on top of
this), and thus, all manners of tools use it, not just hive. For eg., in the
example you use, of ACID tables not being visible, or worse, erroring out if a
user has their capabilities as null, this will then break other existing tools
written against the metastore api, such as tools that do a UI display of the
warehouse (data explorers/etc), or will break existing wh management tools
which set table properties for expiry/cleanup, etc. They can be updated(with
recompiling), by adding capabilities to each explicitly, but that then becomes
a moving target for them unless we define something like ALL, at which point
everyone starts using ALL and the point gets lost. On the other hand,
warehouses that have tools like that could also disable the compatibility check
altogether, and it's a good thing you include that - this allows them to
continue unbroken. But.. that then causes issues if we write code that then
necessarily depends on the compatibility check acting as a gate.
Thus, while this is a useful capability, the possibility for accidental misuse
leading to breaking backward compatibility is high. I would still like this to
be introduced, however, but maybe with more documented warnings on usage, about
why one must be careful of implementing this?
was (Author: sushanth):
I like the fundamental idea, and I think this makes sense. I would suggest a
couple of things, though.
a) checkClient is not really handling "checking" client compatibility. Instead,
it is instead acting as a gating assert for entering sections of code where we
make an assumption that feature exists. I think that an api style that did the
following would be more usable:
{code}
private boolean checkClientCompatible(ClientCapabilities capabilities,
ClientCapability value, String what, String call) {
// return true if compatible, false, if not. Simply test, no more.
...
}
private boolean assertClientCompatible(ClientCapabilities capabilities,
ClientCapability value, String what, String call) throws
MetaException {
if (!checkClientCompatible(capabilities, value, what, call) {
// throw exception similar to existing checkClient
}
}
{code}
Then, this allows us to assert that something should be compatible if it
requires it, and allows us to write backward compatible code if possible by
checking capability.
Also, I would advise cautionary use of the capabilities notion, since the
metastore api is a public api (also, the HCatClient api that sits on top of
this), and thus, all manners of tools use it, not just hive. For eg., in the
example you use, of ACID tables not being visible, or worse, erroring out if a
user has their capabilities as null, this will then break other tools written
against the metastore api, such as tools that do a UI display of the warehouse
(data explorers/etc), or will break existing wh management tools which set
table properties for expiry/cleanup, etc.
Thus, while this is a useful capability, the possibility for accidental misuse
leading to breaking backward compatibility is high. I would still like this to
be introduced, however, but maybe with more documented warnings on usage, about
why one must be careful of implementing this?
> create backward compat checking for metastore APIs
> --------------------------------------------------
>
> Key: HIVE-15062
> URL: https://issues.apache.org/jira/browse/HIVE-15062
> Project: Hive
> Issue Type: Bug
> Reporter: Sergey Shelukhin
> Assignee: Sergey Shelukhin
> Attachments: HIVE-15062.01.nogen.patch, HIVE-15062.01.patch,
> HIVE-15062.02.nogen.patch, HIVE-15062.02.patch, HIVE-15062.03.nogen.patch,
> HIVE-15062.03.patch, HIVE-15062.nogen.patch, HIVE-15062.patch
>
>
> This is to add client capability checking to Hive metastore.
> This could have been used, for example, when introducing ACID tables - a
> client trying to get_table on such a table without specifying that it is
> aware of ACID tables would get an error by default.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
