[GitHub] [iceberg] pvary commented on a diff in pull request #6648: Hive: Refactor commit lock mechanism from HiveTableOperations

["pvary (via GitHub)" <gi...@apache.org>]

pvary commented on code in PR #6648:
URL: https://github.com/apache/iceberg/pull/6648#discussion_r1094266424


##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveLock.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+public interface HiveLock {
+  void lock() throws LockException;
+
+  void ensureActive() throws LockException;
+
+  void unlock();
+}

Review Comment:
   I did some drafting myself to see how hard would this be, and here are my thoughts:
   - There is a big difference between the `LockManager` which aims to be the single shared instance which is called by multiple `TableOperations` instances (created by the same `Catalog`), and the `MetastoreLock` which is handling a single instance of the lock. With the current code it is possible to have a different locking configuration  for different tables created through the same HiveCatalog implementation (with the use of `HiveCatalog.setConf`), and we aim to keep that, so some tables will use the old locking and some tables will use the new locking mechanism (See: #6570).
   
   For the concrete methods:
   - `ensureActive` - this is currently Hive specific, but if you need to have a heartbeat then it means that you run the risk that one way or another the heartbeat fails (temporary connection issue, or whatever). I think this means that failing heartbeat is not Hive specific, just an issue not addressed by other LockManager instances (maybe not that pronounced there but still a valid issue)
   - `LockManager`
      - `acquire` HiveLock needs `database` and `table` to acquire a lock - whereas LockManager has only `entityId` - we can get over with it by splitting the `entityId` on `.`, but this is not that nice
      - `release` - we can use this
      - `initialize` - we can get away with pushing the needed stuff through the constructor - but remember that we have to create the LockManager for every operation to ensure that the configurations are honored.
   - `BaseLockManager` only implements the following methods:
      - `scheduler` - needs fixing
      - `initialize` - only reads the cofigurations which are not needed anyway for Hive as we plan to keep the old config
   
   So as I see now, the Hive lock handling would not benefit too much from the existing `LockManager` infra.
   
   All-in-all this would be another big change above this big refactor.
   If we want to do this we should split it out to multiple steps to help the reviewers.
   Also, I am overwhelmed with other work, so I will not have too much time working on this.
   
   @amogh-jahagirdar: I propose to merge this change for now, and if you have time I would be happy to review your PR from the Hive perspective which merges the 2 Locking behaviour.
   
   Would that work for you @amogh-jahagirdar?
   
   Thanks,
   Peter 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org