HIVE-28822: Concurrent INSERTs can silently lose rows or fail with FileAlreadyExistsException on S3 (non-ACID) - #6642
Open
abstractdog wants to merge 10 commits into
Open
Conversation
Member
|
@abstractdog recent fix in this area: 5ae5a70 cc @difin |
Contributor
Author
yeah, I confirmed that it didn't solve the problem I was investigating completely so that's why I would really appreciate a review on this patch from you guys :) |
abstractdog
force-pushed
the
HIVE-28822-concurrent-insert-fix
branch
from
July 28, 2026 06:27
c4ab838 to
977579c
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
abstractdog
force-pushed
the
HIVE-28822-concurrent-insert-fix
branch
2 times, most recently
from
July 28, 2026 10:03
4f81258 to
71f6fe8
Compare
Contributor
Author
none of the issues was introduced by this patch, this also fixed brain method problem by refactoring logic to a new method |
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
Member
|
do we need to fix Utilities.moveFile as well ? |
Contributor
Author
the same pattern, yes, created follow-up ticket about that: https://issues.apache.org/jira/browse/HIVE-29775 |
abstractdog
force-pushed
the
HIVE-28822-concurrent-insert-fix
branch
from
July 28, 2026 13:26
aab3c3b to
7a10a00
Compare
Member
|
The tag is per-query, but a single query can move multiple files with the same basename into the same destination directory, isn't it? FS On master, the exists-probe loop resolves it (000000_0 + 000000_0_copy_1); under the PR both legs compute the same 000000_0_copy_ and collide HIVE-21100 seems to add branch index, so we might be sorted |
deniskuzZ
reviewed
Jul 28, 2026
deniskuzZ
reviewed
Jul 28, 2026
| "(?:_copy_([0-9]{1,6}))?" + // copy file index | ||
| "(\\d+)" + // taskId | ||
| "(?:_(\\d{1,6}))?" + // _<attemptId> (limited to 6 digits) | ||
| "(?:_copy_(\\d{1,6}|[\\da-fA-F]{8}))?" + // copy suffix: numeric counter, or 8-hex uniqueness tag |
Member
There was a problem hiding this comment.
won't we fail on convertion non-ACID managed table to ACID ? AcidUtils.ORIGINAL_PATTERN_COPY won't match
Contributor
Author
There was a problem hiding this comment.
good catch, need to check
Contributor
Author
There was a problem hiding this comment.
valid concern, it was addressed by changing the pattern, also introduced unit tests that failed without properly patching this, I saw 2 different exceptions:
ERROR : DDLTask failed, DDL Operation: class org.apache.hadoop.hive.ql.ddl.table.misc.properties.AlterTableSetPropertiesOperation
org.apache.hadoop.hive.ql.metadata.HiveException: Unable to alter table. java.lang.IllegalStateException: Unexpected data file name format. Cannot convert default.t_acid_demo to transactional table. File: s3a://dw-team-bucket/tmp/t_acid_demo/000000_0_copy_f0796c02aef8435d
at org.apache.hadoop.hive.ql.metadata.Hive.alterTable(Hive.java:1007)
at org.apache.hadoop.hive.ql.metadata.Hive.alterTable(Hive.java:943)
at org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableOperation.finalizeAlterTableWithWriteIdOp(AbstractAlterTableOperation.java:163)
at org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableOperation.execute(AbstractAlterTableOperation.java:82)
at org.apache.hadoop.hive.ql.ddl.DDLTask.execute(DDLTask.java:84)
at org.apache.hadoop.hive.ql.exec.Task.executeTask(Task.java:214)
at org.apache.hadoop.hive.ql.exec.TaskRunner.runSequential(TaskRunner.java:105)
at org.apache.hadoop.hive.ql.Executor.launchTask(Executor.java:354)
at org.apache.hadoop.hive.ql.Executor.launchTasks(Executor.java:327)
at org.apache.hadoop.hive.ql.Executor.runTasks(Executor.java:244)
at org.apache.hadoop.hive.ql.Executor.execute(Executor.java:105)
at org.apache.hadoop.hive.ql.Driver.execute(Driver.java:346)
at org.apache.hadoop.hive.ql.Driver.runInternal(Driver.java:191)
at org.apache.hadoop.hive.ql.Driver.run(Driver.java:143)
at org.apache.hadoop.hive.ql.Driver.run(Driver.java:138)
at org.apache.hadoop.hive.ql.reexec.ReExecDriver.run(ReExecDriver.java:190)
at org.apache.hive.service.cli.operation.SQLOperation.runQuery(SQLOperation.java:234)
at org.apache.hive.service.cli.operation.SQLOperation$BackgroundWork$1.run(SQLOperation.java:334)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
at java.base/javax.security.auth.Subject.doAs(Subject.java:525)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1953)
at org.apache.hive.service.cli.operation.SQLOperation$BackgroundWork.run(SQLOperation.java:354)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
at java.base/java.lang.Thread.run(Thread.java:1583)
and:
Caused by: java.lang.IllegalArgumentException: Bucket ID out of range: -1
at org.apache.hive.com.google.common.base.Preconditions.checkArgument(Preconditions.java:134)
at org.apache.hadoop.hive.ql.io.BucketCodec$2.encode(BucketCodec.java:103)
at org.apache.hadoop.hive.ql.io.orc.VectorizedOrcAcidRowBatchReader.computeOffsetAndBucket(VectorizedOrcAcidRowBatchReader.java:797)
at org.apache.hadoop.hive.ql.io.orc.OrcInputFormat$SplitGenerator.callInternal(OrcInputFormat.java:1548)
at org.apache.hadoop.hive.ql.io.orc.OrcInputFormat$SplitGenerator$1.run(OrcInputFormat.java:1535)
at org.apache.hadoop.hive.ql.io.orc.OrcInputFormat$SplitGenerator$1.run(OrcInputFormat.java:1532)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
at java.base/javax.security.auth.Subject.doAs(Subject.java:525)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1953)
at org.apache.hadoop.hive.ql.io.orc.OrcInputFormat$SplitGenerator.call(OrcInputFormat.java:1532)
at org.apache.hadoop.hive.ql.io.orc.OrcInputFormat$SplitGenerator.call(OrcInputFormat.java:1348)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
... 3 more
TestInsertCopySuffixOnFakeS3.java extensively tests different source tables converted to ACID, also acid_convert_16hex_copy_tag.q was added for the same
asf-ci-hive
added
tests failed
tests pending
tests passed
and removed
tests pending
tests failed
labels
deniskuzZ
reviewed
Aug 12, 2026
| "(?:_copy_([0-9]{1,6}))?" + // copy file index | ||
| "(\\d+)" + // taskId | ||
| "(?:_(\\d{1,6}))?" + // _<attemptId> (limited to 6 digits) | ||
| "(?:_copy_(\\d{1,6}|[\\da-fA-F]{16}))?" + // copy suffix: numeric counter, or 16-hex uniqueness tag |
Member
There was a problem hiding this comment.
minor: AcidUtils uses 0-9a-fA-F]{16}. maybe let's keep [0-9] instead of '\d' for consistency
deniskuzZ
reviewed
Aug 12, 2026
deniskuzZ
reviewed
Aug 12, 2026
| * The shape matches {@link ParsedOutputFileName}'s copy-index group so downstream filename | ||
| * parsing (taskId, attemptId, copyIndex) keeps working. | ||
| */ | ||
| static String computeUniquenessTag(HiveConf conf) { |
Member
There was a problem hiding this comment.
could we add extractUniquenessTag overload in QueryPlan?
QueryPlan.extractUniquenessTag(conf)
deniskuzZ
reviewed
Aug 12, 2026
deniskuzZ
reviewed
Aug 12, 2026
| * taskIds, copy/copyFromLocal do not race on the destination filename, and overwrite explicitly | ||
| * clears the target first. | ||
| */ | ||
| private static Path pickDestFilePath(HiveConf conf, FileSystem sourceFs, Path sourcePath, FileSystem destFs, |
Member
There was a problem hiding this comment.
should we extract this into MoveTask or something? make static util?
Contributor
Author
There was a problem hiding this comment.
not sure, pickDestFilePath is so tightly coupled to Hive.mvFile, they also share almost the same huge method signature
private static Path mvFile(HiveConf conf, FileSystem sourceFs, Path sourcePath, FileSystem destFs, Path destDirPath,
boolean isSrcLocal, boolean isOverwrite, boolean isRenameAllowed,
int taskId) throws IOException {
Path destFilePath = pickDestFilePath(conf, sourceFs, sourcePath, destFs, destDirPath, taskId, isOverwrite,
isRenameAllowed);
pickDestFilePath helps reduce the body of mvFile, but alone it's just a static method, which wants to be a useful utility method, but it isn't; I cannot see the value of refactoring it, that's why I kept it as "private static", let me know if you want me to refactor it
deniskuzZ
reviewed
Aug 12, 2026
| // uniqueness tag (non-atomic-rename FS such as S3A: _copy_<queryTag>). | ||
| private static final Pattern ORIGINAL_PATTERN_COPY = | ||
| Pattern.compile("[0-9]+_[0-9]+" + "_copy_" + "[0-9]+"); | ||
| Pattern.compile("[0-9]+_[0-9]+" + "_copy_" + "(?:[0-9]{1,6}|[0-9a-fA-F]{16})"); |
Member
There was a problem hiding this comment.
is this the same as in AcidUtils? could we reuse?
Contributor
Author
There was a problem hiding this comment.
I wish, but standalone-metastore doesn't depend on the rest of hive, so we cannot refer to AcidUtils here (so keep on doing the "having everything twice" for years :) )
…tly lose rows or fail with FileAlreadyExistsException on S3 (non-ACID)
On file systems whose rename is not atomic-if-absent (S3A and other object
stores), two concurrent non-ACID INSERTs that create the same new dynamic
partition race in Hive.mvFile between the exists()-driven _copy_N picker
and the destFs.rename() call. Depending on the timing this shows up as
either:
* Fail-loud — S3AFileSystem.initiateRename throws
FileAlreadyExistsException, surfacing to the client as
"MoveTask return code 40000". This matches the customer report:
[load-dynamic-partitionsToAdd-0] Failed to move: ...
Caused by: FileAlreadyExistsException:
Failed to rename .../000001_N to .../000001_N_copy_M;
destination file exists
at S3AFileSystem.initiateRename
at Hive.mvFile
at Hive.copyFiles
at Hive.loadPartitionInternal
at Hive.lambda$loadDynamicPartitions
* Fail-silent — both writers' internal exists() probes see the target
as not-yet-present, both PUTs go to the same key, and the second
silently overwrites the first (last writer wins, no error surfaces).
Reproduced with 30 concurrent `insert into p_test values (i,2)` against
an S3-backed external Parquet table: 2 sessions fail with MoveTask, 6
rows silently missing, and the final S3 listing shows the same _copy_N
slot claimed by multiple writers.
Fix: on filesystems in UnstableRenameFileSystem (S3A/S3N/S3/GS today), the
copy suffix in Hive.mvFile carries a per-query 8-hex uniqueness tag
(derived from hive.query.id) *in place of* the numeric counter. Two
concurrent writers land at distinct destinations —
basename_copy_<queryTag1>
basename_copy_<queryTag2>
— so there is no picker loop and no rename race. On stable-rename
filesystems (HDFS, local) the historical numeric _copy_N picker is
preserved unchanged. UnstableRenameFileSystem is an in-code enum rather
than a configuration knob: the set of unsafe filesystems is a property
of the filesystem impl, not something an operator should override.
ParsedOutputFileName's copy-index regex group is widened from
`[0-9]{1,6}` to `[0-9]{1,6}|[0-9a-fA-F]{8}` so both shapes parse.
getCopyIndex returns either the numeric counter or the 8-hex tag
verbatim; downstream taskId / attemptId extraction is unaffected.
The ACID branch (taskId != -1) and the isOverwrite branch are unchanged
— ACID writers already own unique taskIds, and overwrite explicitly
clears the target first.
If a future unstable-rename filesystem is ever missed by the enum, the
failure mode is the same loud FileAlreadyExistsException →
MoveTask return code 40000 that we surface today — a correct, actionable
signal rather than a silent loss.
Verification:
* unit: TestHiveCopyFiles.testUniquenessTagAndUnstableFsGating covers
the enum recognition (matches on s3a/s3n/s3/gs, rejects hdfs/file)
and the per-query tag shape (distinct queryIds → distinct 8-hex
tags, empty queryId → empty tag). ParsedOutputFileNameTest gains
3 cases: a copy suffix that is an 8-hex tag (plain and with
extension), and a strict-shape check that rejects 7-char / non-hex
forms. All 31 tests green (20 in TestHiveCopyFiles under 4
parameterizations + 11 in ParsedOutputFileNameTest).
* end-to-end: 30-way concurrent burst against s3a://... table:
Before: 24 rows persisted, 2 MoveTask failures, many _copy_N.
After: 30 rows persisted, 0 MoveTask failures, 30 distinct
000001_N_copy_<hex> keys in S3, no FAEE.
Co-Authored-By: Claude <noreply@anthropic.com>
…nessTag; widen to 16 hex
Move the per-query uniqueness-tag helper out of Hive.mvFile's neighborhood
and into QueryPlan, next to makeQueryId() which produces the queryId shape
the tag is derived from. Widen the tag from 8 hex chars (the upper 32 bits
of the UUID's most-significant half, taken via substring) to 16 hex chars
(the full 64-bit most-significant half, taken via UUID.fromString +
getMostSignificantBits) — 16 hex is well-formed hex regardless of how
QueryPlan.makeQueryId's UUID rendering evolves, and 2^64 headroom makes
birthday-collisions vanishingly rare for any realistic per-partition
concurrency.
Layout:
* QueryPlan.extractUniquenessTag(String queryId): public static helper
that parses the UUID at the tail of the queryId and returns
String.format("%016x", uuid.getMostSignificantBits()).
* Hive.computeUniquenessTag(HiveConf): reads hive.query.id, guards
null/empty, delegates to QueryPlan.extractUniquenessTag.
* ParsedOutputFileName's copy-index regex group widens from
{[0-9a-fA-F]{8}} to {[0-9a-fA-F]{16}} so downstream filename parsing
(taskId, attemptId, copyIndex) keeps working. Numeric _copy_N form
unchanged.
* Tests updated: ParsedOutputFileNameTest exercises the 16-hex shape
and the strict-shape rejection at 15 chars / non-hex chars.
TestHiveCopyFiles.testUniquenessTagAndUnstableFsGating asserts the
exact 16-hex value produced from two known UUIDs (f47ac10b58cc4372
and 9c8a44f1e2b34a1c).
End-to-end verification: 30-way concurrent `insert into p_test values
(i,2)` against an S3-backed external table produced 30 rows, 30 distinct
000001_N_copy_<16-hex> files, zero MoveTask failures, zero FAEE.
Co-Authored-By: Claude <noreply@anthropic.com>
abstractdog
force-pushed
the
HIVE-28822-concurrent-insert-fix
branch
from
August 12, 2026 11:58
5181edf to
d19d399
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
On filesystems in UnstableRenameFileSystem (S3A/S3N/S3/GS today), the copy suffix in Hive.mvFile carries a per-query 8-hex uniqueness tag (derived from hive.query.id) in place of the numeric counter. Two concurrent writers land at distinct destinations —
basename_copy_
basename_copy_
— so there is no picker loop and no rename race. On stable-rename filesystems (HDFS, local) the historical numeric _copy_N picker is preserved unchanged. UnstableRenameFileSystem is an in-code enum rather than a configuration knob: the set of unsafe filesystems is a property of the filesystem impl, not something an operator should override.
ParsedOutputFileName's copy-index regex group is widened from
[0-9]{1,6}to[0-9]{1,6}|[0-9a-fA-F]{8}so both shapes parse. getCopyIndex returns either the numeric counter or the 8-hex tag verbatim; downstream taskId / attemptId extraction is unaffected.The ACID branch (taskId != -1) and the isOverwrite branch are unchanged — ACID writers already own unique taskIds, and overwrite explicitly clears the target first.
If a future unstable-rename filesystem is ever missed by the enum, the failure mode is the same loud FileAlreadyExistsException → MoveTask return code 40000 that we surface today — a correct, actionable signal rather than a silent loss.
Why are the changes needed?
On file systems whose rename is not atomic-if-absent (S3A and other object stores), two concurrent non-ACID INSERTs that create the same new dynamic partition race in Hive.mvFile between the exists()-driven _copy_N picker and the destFs.rename() call. Depending on the timing this shows up as either:
Fail-loud — S3AFileSystem.initiateRename throws FileAlreadyExistsException, surfacing to the client as "MoveTask return code 40000". This matches the customer report:
[load-dynamic-partitionsToAdd-0] Failed to move: ...
Caused by: FileAlreadyExistsException:
Failed to rename .../000001_N to .../000001_N_copy_M;
destination file exists
at S3AFileSystem.initiateRename
at Hive.mvFile
at Hive.copyFiles
at Hive.loadPartitionInternal
at Hive.lambda$loadDynamicPartitions
Fail-silent — both writers' internal exists() probes see the target as not-yet-present, both PUTs go to the same key, and the second silently overwrites the first (last writer wins, no error surfaces).
Reproduced with 30 concurrent
insert into p_test values (i,2)against an S3-backed external Parquet table: 2 sessions fail with MoveTask, 6 rows silently missing, and the final S3 listing shows the same _copy_N slot claimed by multiple writers.Does this PR introduce any user-facing change?
It depends on whether the actual user is interested in the underlying file structure (not directory, but files).
Pre-patch, the unstable result of a highly concurrent INSERT INTO scenario was something like below: be mindful of 30 insert operations vs. 24 result files, this is just one of the symptoms I referred to as "Fail-silent" above:
After the patch, it becomes:
How was this patch tested?
unit: TestHiveCopyFiles.testUniquenessTagAndUnstableFsGating covers the enum recognition (matches on s3a/s3n/s3/gs, rejects hdfs/file) and the per-query tag shape (distinct queryIds → distinct 8-hex tags, empty queryId → empty tag). ParsedOutputFileNameTest gains 3 cases: a copy suffix that is an 8-hex tag (plain and with extension), and a strict-shape check that rejects 7-char / non-hex forms. All 31 tests green (20 in TestHiveCopyFiles under 4 parameterizations + 11 in ParsedOutputFileNameTest).
end-to-end: 30-way concurrent burst against s3a://... table: Before: 24 rows persisted, 2 MoveTask failures, many copy_N. After: 30 rows persisted, 0 MoveTask failures, 30 distinct 000001_N_copy keys in S3, no FAEE.
Disclaimer: end-to-end testing was done by Claude after I made a following small testing infra available for it:
post-patch: