Hi Peter Thanks for bringing this issue up and thanks for working on it as well. I haven't experienced these problems first-hand, so don't have opinion yet on what the solution should or shouldn't be.
With this new approach, is there any plan to address situations where more than one application or application version is writing to a metastore? In such scenario, some applications may be updated to the new commit mechanism and some may not, leading to locks being ineffective (=> data corruption). Or, is the assumption that in a given deployment all applications are switched to the new way at the same time? It's awesome that you shared all these links for the context. For even more context, the only think I can add is that Trino has it's own, Iceberg-compatible, implementation of Hive Catalog and the table lock is acquired here: https://github.com/trinodb/trino/blob/9feb3d1fb13e0374a17c3f96535342872aa19be5/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java#L68-L72 Best PF On Mon, Jan 16, 2023 at 2:18 PM Péter Váry <peter.vary.apa...@gmail.com> wrote: > Hi Team, > > I have seen several complaints when HiveCatalog was used and the locks > were not removed correctly [1], [2] and suggested solutions [3]. Also did > my share to fix the situation as much as possible [4]. The issue is that > every fix is just a band-aid when the client can disappear and leave behind > an abandoned lock, this way preventing further changes on the given Iceberg > table. > > To fix this I propose the following solution: > 1. We can provide a minimal Hive feature to check the previous metadata > location transactionally before altering a table and setting the new > snapshot > 2. We can have a configuration to turn off using the Hive locks in the > HiveCatalog and we can rely on the new Hive feature to provide us the > atomicity which is needed for an Iceberg commit > > I have created the above mentioned minimal Hive feature request > (HIVE-26882 [5]) and the PR to fix it [6] also, with the approval of the > Hive community backported and merged this feature to the active branches: > branch-3 [7], branch-3.1 [8], branch-2 [9], branch-2.3 [10]. This means > that any of the next Hive releases will contain this fix, and any company > having their own forks could easily cherry-pick this minimal change. The > first upcoming upstream Hive release with this feature would be the Hive > 3.2.0 which is expected to come around 2-3 months [11]. > > Also I have created the Iceberg PR to start using the new feature [12]. > This has 2 parts: > 1. Rebased Adam Szita's work [13] which refactored out the Lock related > features on a single class > 2. Added a small patch which adds the possibility to use the new feature > and turn of the locking [14] for HiveCatalog tables > > Using the new feature would be beneficial in multiple ways: > - Enhances stability of the writers > - Improves the performance of the commits, as fewer number of HMS calls > are needed for a commit operation > > My question for the community is: > - Do we want to merge both PRs to the Iceberg code, or do we want to merge > only the refactor PR (1) to the Iceberg code and only merge the PR with the > new configuration (2) after there is an Apache Hive release with the new > Hive feature? > > I would vote for merging the configuration PR too because of the following > reasons: > - Many companies are using their own fork of Hive - they do not need to > wait for the Apache release to add a new feature > - The PR itself is small, and by default it is turned off - will not cause > to many issues with maintaining the code > - We will not forget to add the change when the Apache Hive is released > > What are your thoughts? > > If you have time, please review the PR. > > Thanks, > Peter > > [1] Should do the best efforts to release hive table lock #3336 - > https://github.com/apache/iceberg/pull/3336 > [2] Lock remains in HMS if HiveTableOperations gets killed (direct process > shutdown - no signals) after lock is acquired #2301 - > https://github.com/apache/iceberg/pull/2301 > [3] What is the purpose of Hive Lock ? #6370 - > https://github.com/apache/iceberg/issues/6370 > [4] Hive: Lock hardening #6451 - > https://github.com/apache/iceberg/pull/6451 > [5] HIVE-26882: Allow transactional check of Table parameter before > altering the Table - https://issues.apache.org/jira/browse/HIVE-26882 > [6] master/HIVE-26882: https://github.com/apache/hive/pull/3888 > > [7] branch-3/HIVE-26882: https://github.com/apache/hive/pull/3943 > > [8] branch-3.1/HIVE-26882: : https://github.com/apache/hive/pull/3944 > > [9] branch-2/HIVE-26882: : https://github.com/apache/hive/pull/3946 > > [10] branch-2.1/HIVE-26882: : https://github.com/apache/hive/pull/3947 > > [11] Upcoming Hive release: > https://issues.apache.org/jira/browse/HIVE-26882?focusedCommentId=17677002&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17677002 > > [12] Hive: Use EnvironmentContext instead of Hive Locks to provide > transactional commits after HIVE-26882 - > https://github.com/apache/iceberg/pull/6570 > > [13] Refactor commit lock mechanism from HiveTableOperations #5877 - > https://github.com/apache/iceberg/pull/5877 > > [14] Hive: Use EnvironmentContext instead of Hive Locks to provide > transactional > commits after HIVE-26882 - > https://github.com/apache/iceberg/pull/6570/commits/dd38a44fb4aa9c6423a7474e9d99258b38a2e609 > > >
