Don't pass expected_parameter_key/value with Hive Locking enabled #26358
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Laith Alzyoud.
|
laithalzyoud
changed the title
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Laith Alzyoud.
|
laithalzyoud
force-pushed
the
fix-hms-environment-context-using-locks
branch
from
1a3fd52 to
82798cf
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
| Table updatedTable = tableUpdateFunction.apply(table, newMetadataLocation); | ||
|
|
||
| // it's redundant to pass metadata_location to environmentContext if Hive locking is enabled | ||
| Map<String, String> environmentContext = lockingEnabled ? ImmutableMap.of() : environmentContext(metadataLocation); |
There was a problem hiding this comment.
I don't know anything about this code, but is there a test we can update or add a new one?
ebyhr
force-pushed
the
fix-hms-environment-context-using-locks
branch
from
82798cf to
b38361b
Compare
|
|
||
| Table updatedTable = tableUpdateFunction.apply(table, newMetadataLocation); | ||
|
|
||
| // Passing environment context causes redundant operations if Hive locking is enabled |
There was a problem hiding this comment.
Could you give more details about the consequences that what is "redundant operations" in the comments?
|
Hey @nineinchnick @chenjian2664! I added some explanation in the related issue #26360, basically this code was originally introduced in #25445 to allow committing to Iceberg tables without using Hive locks by passing the latest metadata file location to Hive, if this is passed to Hive then it will go through extra operations to confirm that the metadata location didn't change since the write/update operation started. In the case when Hive locking is enabled, there is no need to do this extra check on Hive side as Hive locking already guarantees the atomicity of the transaction, therefore it's only needed when Hive locking is disabled. |
|
@laithalzyoud the #25445 added some tests. Can they be updated to cover this change? Thanks for adding the explanation in the issue, but it would be ideal to have a test that would also explain it. |
|
Thanks for merging @ebyhr! |
Description
This PR fixes #26360
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.