Copilot commented on code in PR #6377: URL: https://github.com/apache/hive/pull/6377#discussion_r2963468172
########## standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/BaseHandler.java: ########## @@ -0,0 +1,1029 @@ +/* + * 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.hadoop.hive.metastore.handler; + +import com.codahale.metrics.Counter; +import com.facebook.fb303.FacebookBase; +import com.facebook.fb303.fb_status; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.util.concurrent.Striped; + +import javax.jdo.JDOException; +import java.util.AbstractMap; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.locks.Lock; +import java.util.regex.Pattern; + +import org.apache.commons.collections4.CollectionUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hive.common.TableName; +import org.apache.hadoop.hive.metastore.AcidEventListener; +import org.apache.hadoop.hive.metastore.AlterHandler; +import org.apache.hadoop.hive.metastore.DefaultMetaStoreFilterHookImpl; +import org.apache.hadoop.hive.metastore.FileMetadataManager; +import org.apache.hadoop.hive.metastore.HMSHandler; +import org.apache.hadoop.hive.metastore.HMSHandlerContext; +import org.apache.hadoop.hive.metastore.HMSMetricsListener; +import org.apache.hadoop.hive.metastore.HiveMetaStore; +import org.apache.hadoop.hive.metastore.IHMSHandler; +import org.apache.hadoop.hive.metastore.IMetaStoreMetadataTransformer; +import org.apache.hadoop.hive.metastore.MetaStoreEndFunctionContext; +import org.apache.hadoop.hive.metastore.MetaStoreEndFunctionListener; +import org.apache.hadoop.hive.metastore.MetaStoreEventListener; +import org.apache.hadoop.hive.metastore.MetaStoreFilterHook; +import org.apache.hadoop.hive.metastore.MetaStoreInit; +import org.apache.hadoop.hive.metastore.MetaStoreInitContext; +import org.apache.hadoop.hive.metastore.MetaStoreInitListener; +import org.apache.hadoop.hive.metastore.MetaStoreListenerNotifier; +import org.apache.hadoop.hive.metastore.MetaStorePreEventListener; +import org.apache.hadoop.hive.metastore.PartFilterExprUtil; +import org.apache.hadoop.hive.metastore.PartitionExpressionProxy; +import org.apache.hadoop.hive.metastore.RawStore; +import org.apache.hadoop.hive.metastore.SessionPropertiesListener; +import org.apache.hadoop.hive.metastore.TransactionalMetaStoreEventListener; +import org.apache.hadoop.hive.metastore.TransactionalValidationListener; +import org.apache.hadoop.hive.metastore.Warehouse; +import org.apache.hadoop.hive.metastore.api.CmRecycleRequest; +import org.apache.hadoop.hive.metastore.api.CmRecycleResponse; +import org.apache.hadoop.hive.metastore.api.ConfigValSecurityException; +import org.apache.hadoop.hive.metastore.api.CurrentNotificationEventId; +import org.apache.hadoop.hive.metastore.api.Database; +import org.apache.hadoop.hive.metastore.api.DatabaseType; +import org.apache.hadoop.hive.metastore.api.HiveObjectPrivilege; +import org.apache.hadoop.hive.metastore.api.HiveObjectRef; +import org.apache.hadoop.hive.metastore.api.HiveObjectType; +import org.apache.hadoop.hive.metastore.api.InvalidObjectException; +import org.apache.hadoop.hive.metastore.api.InvalidOperationException; +import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.metastore.api.NoSuchObjectException; +import org.apache.hadoop.hive.metastore.api.NotificationEventRequest; +import org.apache.hadoop.hive.metastore.api.NotificationEventResponse; +import org.apache.hadoop.hive.metastore.api.NotificationEventsCountRequest; +import org.apache.hadoop.hive.metastore.api.NotificationEventsCountResponse; +import org.apache.hadoop.hive.metastore.api.PrincipalType; +import org.apache.hadoop.hive.metastore.api.PrivilegeBag; +import org.apache.hadoop.hive.metastore.api.PrivilegeGrantInfo; +import org.apache.hadoop.hive.metastore.api.Role; +import org.apache.hadoop.hive.metastore.api.TableMeta; +import org.apache.hadoop.hive.metastore.api.Type; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.events.ConfigChangeEvent; +import org.apache.hadoop.hive.metastore.events.PreEventContext; +import org.apache.hadoop.hive.metastore.events.PreReadDatabaseEvent; +import org.apache.hadoop.hive.metastore.messaging.EventMessage; +import org.apache.hadoop.hive.metastore.metrics.Metrics; +import org.apache.hadoop.hive.metastore.metrics.MetricsConstants; +import org.apache.hadoop.hive.metastore.txn.TxnStore; +import org.apache.hadoop.hive.metastore.utils.JavaUtils; +import org.apache.hadoop.hive.metastore.utils.MetaStoreServerUtils; +import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils; +import org.apache.hadoop.hive.metastore.utils.MetastoreVersionInfo; +import org.apache.hadoop.hive.metastore.utils.SecurityUtils; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.util.ReflectionUtils; +import org.apache.thrift.TException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.commons.lang3.StringUtils.isBlank; +import static org.apache.commons.lang3.StringUtils.join; +import static org.apache.hadoop.hive.metastore.ExceptionHandler.handleException; +import static org.apache.hadoop.hive.metastore.ExceptionHandler.newMetaException; +import static org.apache.hadoop.hive.metastore.HMSHandler.createDefaultCatalog; +import static org.apache.hadoop.hive.metastore.HMSHandler.getIPAddress; +import static org.apache.hadoop.hive.metastore.Warehouse.DEFAULT_CATALOG_NAME; +import static org.apache.hadoop.hive.metastore.Warehouse.DEFAULT_DATABASE_COMMENT; +import static org.apache.hadoop.hive.metastore.Warehouse.DEFAULT_DATABASE_NAME; +import static org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars.HIVE_IN_TEST; +import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.CAT_NAME; +import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.DB_NAME; +import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.parseDbName; +import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.prependCatalogToDbName; + +/** + * This class serves as the super class for all handlers that implement the IHMSHandler + */ +public abstract class BaseHandler extends FacebookBase implements IHMSHandler { + private static final Logger LOG = LoggerFactory.getLogger(BaseHandler.class); + private static String currentUrl; + private static volatile Striped<Lock> tablelocks; + + public static final String ADMIN = "admin"; + public static final String PUBLIC = "public"; + + static final int LOG_SAMPLE_PARTITIONS_MAX_SIZE = 4; + static final int LOG_SAMPLE_PARTITIONS_HALF_SIZE = 2; + static final String LOG_SAMPLE_PARTITIONS_SEPARATOR = ","; + + protected final Configuration conf; + protected FileMetadataManager fileMetadataManager; + protected PartitionExpressionProxy expressionProxy; + protected IMetaStoreMetadataTransformer transformer; + protected Warehouse wh; // hdfs warehouse + protected AlterHandler alterHandler; + protected List<MetaStorePreEventListener> preListeners; + protected List<MetaStoreEventListener> listeners; + protected List<TransactionalMetaStoreEventListener> transactionalListeners; + protected List<MetaStoreEndFunctionListener> endFunctionListeners; + protected List<MetaStoreInitListener> initListeners; + protected MetaStoreFilterHook filterHook; + protected boolean isServerFilterEnabled = false; + + protected BaseHandler(String name, Configuration conf) { + super(name); + this.conf = conf; + } + + @Override + public List<TransactionalMetaStoreEventListener> getTransactionalListeners() { + return transactionalListeners; + } + + @Override + public List<MetaStoreEventListener> getListeners() { + return listeners; + } + + @Override + public IMetaStoreMetadataTransformer getMetadataTransformer() { + return transformer; + } + + @Override + public MetaStoreFilterHook getMetaFilterHook() { + return filterHook; + } + + // Make it possible for tests to check that the right type of PartitionExpressionProxy was + // instantiated. + @VisibleForTesting + public PartitionExpressionProxy getExpressionProxy() { + return expressionProxy; + } + + @Override + public void init() throws MetaException { + init(new Warehouse(conf)); + } + + @VisibleForTesting + public void init(Warehouse wh) throws MetaException { + Metrics.initialize(conf); + + initListeners = MetaStoreServerUtils.getMetaStoreListeners( + MetaStoreInitListener.class, conf, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.INIT_HOOKS)); + for (MetaStoreInitListener singleInitListener: initListeners) { + MetaStoreInitContext context = new MetaStoreInitContext(); + singleInitListener.onInit(context); + } + + String alterHandlerName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.ALTER_HANDLER); + alterHandler = ReflectionUtils.newInstance(JavaUtils.getClass( + alterHandlerName, AlterHandler.class), conf); + this.wh = wh; + + initDefaultSchema(); + + preListeners = MetaStoreServerUtils.getMetaStoreListeners(MetaStorePreEventListener.class, + conf, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.PRE_EVENT_LISTENERS)); + preListeners.add(0, new TransactionalValidationListener(conf)); + listeners = MetaStoreServerUtils.getMetaStoreListeners(MetaStoreEventListener.class, conf, + MetastoreConf.getVar(conf, MetastoreConf.ConfVars.EVENT_LISTENERS)); + listeners.add(new SessionPropertiesListener(conf)); + transactionalListeners = new ArrayList<>() {{ + add(new AcidEventListener(conf)); + }}; + transactionalListeners.addAll(MetaStoreServerUtils.getMetaStoreListeners( + TransactionalMetaStoreEventListener.class, conf, + MetastoreConf.getVar(conf, MetastoreConf.ConfVars.TRANSACTIONAL_EVENT_LISTENERS))); + if (Metrics.getRegistry() != null) { + listeners.add(new HMSMetricsListener(conf)); + } + + boolean canCachedStoreCanUseEvent = false; + for (MetaStoreEventListener listener : transactionalListeners) { + if (listener.doesAddEventsToNotificationLogTable()) { + canCachedStoreCanUseEvent = true; + break; + } + } + if (conf.getBoolean(MetastoreConf.ConfVars.METASTORE_CACHE_CAN_USE_EVENT.getVarname(), false) && + !canCachedStoreCanUseEvent) { + throw new MetaException("CahcedStore can not use events for invalidation as there is no " + Review Comment: The MetaException message says "CahcedStore" (typo). Please correct this to "CachedStore" so operators get a clear error message when this configuration is mis-set. ```suggestion throw new MetaException("CachedStore can not use events for invalidation as there is no " + ``` ########## standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java: ########## @@ -1823,4 +1825,22 @@ public static boolean canUpdateStats(Configuration conf, Table tbl) { return true; } + public static List<String> getPartValsFromName(Table t, String partName) + throws MetaException, InvalidObjectException { + Preconditions.checkArgument(t != null, "Table can not be null"); Review Comment: `getPartValsFromName` throws an `IllegalArgumentException` via `Preconditions.checkArgument` when `t` is null, but the method signature only declares `MetaException` / `InvalidObjectException`. Consider throwing `MetaException` (or `InvalidObjectException`) instead so callers that handle the declared exceptions don't get an unexpected unchecked exception. ```suggestion if (t == null) { throw new MetaException("Table can not be null"); } ``` ########## standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/PrivilegeHandler.java: ########## @@ -0,0 +1,687 @@ +/* + * 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.hadoop.hive.metastore.handler; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hive.metastore.RawStore; +import org.apache.hadoop.hive.metastore.Warehouse; +import org.apache.hadoop.hive.metastore.api.GetPrincipalsInRoleRequest; +import org.apache.hadoop.hive.metastore.api.GetPrincipalsInRoleResponse; +import org.apache.hadoop.hive.metastore.api.GetRoleGrantsForPrincipalRequest; +import org.apache.hadoop.hive.metastore.api.GetRoleGrantsForPrincipalResponse; +import org.apache.hadoop.hive.metastore.api.GetTableRequest; +import org.apache.hadoop.hive.metastore.api.GrantRevokePrivilegeRequest; +import org.apache.hadoop.hive.metastore.api.GrantRevokePrivilegeResponse; +import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleRequest; +import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleResponse; +import org.apache.hadoop.hive.metastore.api.HiveObjectPrivilege; +import org.apache.hadoop.hive.metastore.api.HiveObjectRef; +import org.apache.hadoop.hive.metastore.api.HiveObjectType; +import org.apache.hadoop.hive.metastore.api.InvalidObjectException; +import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.metastore.api.NoSuchObjectException; +import org.apache.hadoop.hive.metastore.api.PrincipalPrivilegeSet; +import org.apache.hadoop.hive.metastore.api.PrincipalType; +import org.apache.hadoop.hive.metastore.api.PrivilegeBag; +import org.apache.hadoop.hive.metastore.api.Role; +import org.apache.hadoop.hive.metastore.api.RolePrincipalGrant; +import org.apache.hadoop.hive.metastore.api.Table; +import org.apache.hadoop.hive.metastore.events.PreAuthorizationCallEvent; +import org.apache.thrift.TException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.hadoop.hive.metastore.ExceptionHandler.handleException; +import static org.apache.hadoop.hive.metastore.ExceptionHandler.rethrowException; +import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.getDefaultCatalog; + +/** + * Privilege and Role methods + */ +public abstract class PrivilegeHandler extends TransactionHandler { + private static final Logger LOG = LoggerFactory.getLogger(PrivilegeHandler.class); + + protected PrivilegeHandler(String name, Configuration conf) { + super(name, conf); + } + + @Override + public PrincipalPrivilegeSet get_privilege_set(HiveObjectRef hiveObject, String userName, + List<String> groupNames) throws TException { + firePreEvent(new PreAuthorizationCallEvent(this)); + String catName = hiveObject.isSetCatName() ? hiveObject.getCatName() : getDefaultCatalog(conf); + HiveObjectType debug = hiveObject.getObjectType(); Review Comment: In `get_privilege_set`, the local variable `debug` is assigned but never used. Please remove it (or use it for logging) to avoid dead code and confusion during future maintenance. ```suggestion ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
