HIVE-29690: Fix StackOverflowError when calling get_table_objects_by_name_req API with both tablenames and pattern#6573
Conversation
72c45a7 to
d2d41d7
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
…name_req API with both tablenames and pattern
22e4729 to
99aa65a
Compare
|
|
@deniskuzZ , @zabetak , @zhangbutao , @ayushtkn could someone please check this PR when you get a chance? Thanks. |
| if(req.getTablesPattern() != null) { | ||
| tables = ms.getTableObjectsByName(catName, dbName, tableNames, projectionsSpec, req.getTablesPattern()); | ||
| // case : tableNames is empty -> return empty list | ||
| if (tableNames != null && tableNames.isEmpty()) { |
There was a problem hiding this comment.
Yes. Some UTs were failing without this change.
|
|
||
| // case : tableNames != null and pattern == null -> batched flow | ||
| // case : tableNames != null and pattern != null -> batched flow with pattern filter | ||
| String tablesPattern = req.getTablesPattern(); |
There was a problem hiding this comment.
can we drop these comments and inline thereq.getTablesPattern() usage?
There was a problem hiding this comment.
Sure, will do the same.
| return tables; | ||
| } | ||
|
|
||
| if (req.getTablesPattern() != null && tableNames == null) { |
There was a problem hiding this comment.
I think the original code was designed to support either the pattern or the names list
cc @saihemanth-cloudera, @dengzhhu653
There was a problem hiding this comment.
+1, The API supports either the pattern or the names list but not both at the same time.
There was a problem hiding this comment.
Adding them both looks unnecessary for me, as we can filter the table names by the pattern ahead before reaching Metastore, thus saving the check against the pattern in the database.
@vikramahuja1001 do you know how/when if a user input the two parameters?
There was a problem hiding this comment.
Thanks for reviewing and your feedback @deniskuzZ @saihemanth-cloudera @dengzhhu653 and apologies for later reply on this.
You all are absolutely right that within hive, all the callers of this API are disciplined and only either pass tblNames or tablesPattern, never both(verified through extensive code review). So this combination is never reached in the normal Hive Code path.
However, get_table_objects_by_name_req is a public Thrift API and HMS is used as a standalone metastore by other engines as well, which are not bound by Hive's internal calling conventions. We faced this issue in our prod machine using Spark engine which uses older hive clients, this is how this bug was surfaced.
Also, the original code's response to this combination is a StackOverflowError, which is never a acceptable behaviour for a server-side API regardless of if the combination was intended. The API should either:
- Handle the input gracefully
- Return an InvalidOperationException explaining the behaviour.
The patch takes option 1 to avoid the exception and handle it gracefully.
@dengzhhu653 , filtering names by pattern client-side before calling the metastore is one valid approach, but it shifts the responsibility on to the caller who in case of other engines might not be aware of this restriction. Also since HMS has a pattern matching logic, handling it server side is more defensive and consistent.
5 participants
What changes were proposed in this pull request?
Setting batching on the basis of tableNames rather than tablePattern
Why are the changes needed?
Check JIRA HIVE-29690 for the same.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Tested with a custom test Program mentioned in the JIRA.