[
https://issues.apache.org/jira/browse/HIVE-28145?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106454#comment-18106454
]
Bhanu Chander Vallabaneni commented on HIVE-28145:
--------------------------------------------------
I traced this on {{master}} and can confirm the mechanism, with a note on the
two questions above and
a suggestion that may make the fix much cheaper than changing the isolation
level.
h3. Where the empty fields come from
{{MetaStoreDirectSql#getPartitionsViaPartNames}} batches into
{{getPartitionsByNames}}. The first
query joins PARTITIONS / SDS / SERDES and fully constructs the {{Partition}}
objects into a
{{TreeMap<Long, Partition>}} keyed by PART_ID. Only then does it issue the
one-to-many follow-ups,
six of them, against the {{partIds}} list built from that map —
{{MetaStoreDirectSql}} lines
~1348-1400:
* {{setPartitionParametersWithFilter}} (PARTITION_PARAMS)
* {{setPartitionValues}} (PARTITION_KEY_VALS)
* {{setSDParameters}} (SD_PARAMS)
* {{setSDCols}} (COLUMNS_V2)
* {{setSerdeParams}} (SERDE_PARAMS)
* plus the skewed/sort/bucket column queries
Each goes through {{MetastoreDirectSqlUtils#loopJoinOrderedResult}}, which runs
the query and invokes
an {{ApplyFunc}} once per returned row. For partition values that callback is
simply:
{code:java}
public void apply(Partition t, Object[] fields) {
t.addToValues((String) fields[1]);
}
{code}
So if the PART_ID rows are deleted between the first query and the follow-up,
the query returns no
rows for that partition, the callback never fires, and the object that was
already built in the map
is returned as-is — with an empty values list. Nothing throws and nothing is
logged. The same holds
for each of the other five follow-ups, which is why the reporter sees it across
params, SD, serde and
sort/bucket/skewed columns rather than only in values.
h3. On the two questions
*Which client:* it should not matter. Everything above is server-side in the
direct-SQL read path, so
any client reaching {{get_partitions_by_names}} is exposed,
{{SessionHiveMetaStoreClient}} included.
*Regression:* I cannot establish that from the code alone, and I would not want
to guess. What I can
say is that the split into "build objects, then join the one-to-many tables in
bulk" is deliberate
and load-bearing rather than accidental — the javadoc on
{{loopJoinOrderedResult}} says DataNucleus
"issues queries separately for every object, which is suboptimal". So this is
intrinsic to the
direct-SQL design, not a recent slip, which argues against fixing it by
collapsing the queries.
h3. A cheaper detection point than the isolation level
Raising the transaction isolation level does fix all six follow-ups at once,
but it changes locking
behaviour for every HMS read that goes through this path, which is a large
blast radius for this bug.
There may be a much smaller lever. {{loopJoinOrderedResult}} already returns
the number of rows the
query produced:
{code:java}
static <T> int loopJoinOrderedResult(PersistenceManager pm, TreeMap<Long, T>
tree,
String queryText, Object[] parameters, int keyIndex, ApplyFunc<T> func)
{code}
and *every* caller currently discards that value. A partition of a partitioned
table always has at
least one PARTITION_KEY_VALS row, so zero rows returned for a PART_ID is an
unambiguous signal that
the partition disappeared mid-read. Threading that count back from
{{setPartitionValues}} would let
{{getPartitionsByNames}} drop those partitions from the result instead of
returning half-built
objects — which is also what a serializable read would have produced, since the
caller asked for
partitions by name and those names no longer exist.
That keeps the change inside the direct-SQL read path and leaves isolation
semantics alone. It does
mean the result can be shorter than the requested name list, so it is worth
confirming that callers
of {{get_partitions_by_names}} tolerate that — a concurrently dropped partition
can already be absent
today if it is deleted before the first query runs, so I would expect them to.
Happy to implement whichever direction the maintainers prefer — the
detection-and-omit approach
above, or the isolation-level change if you would rather fix all the
multi-query APIs in one place.
Could a committer confirm the preferred direction?
> getPartitionsByNames API returns partition objects with empty values in many
> fields when it is executed concurrently with dropPartition API
> --------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HIVE-28145
> URL: https://issues.apache.org/jira/browse/HIVE-28145
> Project: Hive
> Issue Type: Bug
> Reporter: Venugopal Reddy K
> Priority: Major
> Labels: hive-4.1.1-must
>
> *Description:*
> getPartitionsByNames API returns partition objects with empty values in many
> fields when it is executed concurrently with dropPartition API.
> org.apache.hadoop.hive.metastore.MetaStoreDirectSql#getPartitionsViaPartNames
> method does multiple queries to backend db to populate the various fields in
> the partition object. First it queries for part ids using partition names,
> then joins PARTITIONS, SDS, SERDES tables for those part ids and creates
> partition objects. Then another query to PARTITION_KEY_VALS table to get the
> partition values for those part ids and populates in already created
> partition objects.
> So if the partition is deleted just before PARTITION_KEY_VALS table query, it
> can lead to empty values in partition object. This issue can happen for other
> fields(like, partition params, storage descriptor params, serde params, sort
> cols, bucket cols, skewed cols etc) too in partition object that require
> queries to populate those fields.
> *Note: Issue can be observed with both directsql and JDO based query. Need
> to check for all APIs that involves multiple queries to backend database
> within a transaction.*
> *Root Cause:*
> Transaction is opened with default isolation level(read-committed). The
> default in DataNucleus is read-committed.
> *Steps to reproduce:*
> # Create a partitioned table and add 500~1000 dynamic partitions(can add
> dummy partition param, sd param, serde param).
> # Create a thread pool of size 2 and submit 2 tasks. One task to submit
> getPartitionsByNames and another task to submit dropPartition in loop
> # Verify the fields in partition objects returned from
> getPartitionsByNames().
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
