amogh-jahagirdar commented on code in PR #6648: URL: https://github.com/apache/iceberg/pull/6648#discussion_r1091134137
########## 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: @pvary sorry for the late reply so here are my thoughts: > 1.) Shall we keep the LockManager as it is and create/use HiveLockManager with extra methods, or add ensureAcitve method to the LockManger interface? So after reading the code it seems like LockManager was designed in a manner where the heartbeating is assumed to be happening under the hood for the implementations which require heartbeats. If I'm not mistaken ensureActive is used at certain points to check the heartbeat status which is specific for HiveLock. What we could do here is have the separate HiveLockManager interface but it still extends BaseLockManager so we inherit acquire/release, and then the ensureActive is specific to HiveLockManager. We shouldn't have to have a separate lock/unlock imo because acquire/release are already on the LockManager itself, and those are the same operations. let me know if that makes sense! > 2.) Shall we keep the current config for the hive locks or shall we use the new ones instead, or keep both configs and create a deprecation handling for it? I'm not super opinionated on this tbh, the current ones make sense to me. If we do decide to add new ones, having a deprecation path then should happen, rather than changing it all at once. > 3.) Shall we keep the LockManager as it is and create/use HiveLockManager with extra methods, or add ensureAcitve method to the LockManger interface? >The current LockManager implementation silently accepts the job, and does not run the heartbeat until there is an empty thread. HiveTableOperations has its own thread and handles this correctly. This seems like incorrect behavior in the BaseLockManager then, I think we should always guarantee that if the heartbeat thread cannot start we fail. So I think ideally if possible we fix it in the BaseLockmanager itself. In my mind we also add a method `withHeartbeatTask(Runnable heartbeatTask)`. I think @jackye1995 did the original implementation for BaseLockManager, I'll wait for him to comment though if the silent acceptance of the heartbeat thread is intentional. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
