HIVE-19488 : enable CM root based on db parameter, identifying db as a source of replication #345

maheshk114 · 2018-05-10T17:08:59Z

…

sankarh · 2018-05-15T02:39:52Z

ql/src/java/org/apache/hadoop/hive/ql/parse/MetaDataExportListener.java

@@ -90,11 +92,9 @@ private void export_meta_data(PreDropTableEvent tableEvent) throws MetaException
      EximUtil.createExportDump(fs, outFile, mTbl, null, null,
          new HiveConf(conf, MetaDataExportListener.class));
      if (moveMetadataToTrash == true) {
-        wh.deleteDir(metaPath, true);
+        wh.deleteDir(metaPath, true, Hive.get().getDatabase(tbl.getDbName()));


Here we are deleting the metaPath which is the export dir not warehouse location. So, last argument should be false always.

sankarh · 2018-05-15T02:40:41Z

ql/src/java/org/apache/hadoop/hive/ql/parse/MetaDataExportListener.java

-    } catch (IOException e) {
-      throw new MetaException(e.getMessage());
-    } catch (SemanticException e) {
+    } catch (IOException | MetaException | HiveException e) {


Why wrapping MetaException over another MetaException? Shall remove it from catch block.

sankarh · 2018-05-15T02:42:24Z

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java

@@ -349,7 +351,9 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti

    for (Path dead : filesToDelete) {
      LOG.debug("Going to delete path " + dead.toString());
-      replChangeManager.recycle(dead, ReplChangeManager.RecycleType.MOVE, true);
+      if (ReplChangeManager.isReplPolicySet(Hive.get().getDatabase(ci.dbname))) {


Can we get the database object outside the loop and store it in local variable?

sankarh · 2018-05-15T02:45:59Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java

              LOG.info("Partition directory rename from " + srcPath + " to " + destPath + " done.");
              dataWasMoved = true;
            }
          } catch (IOException e) {
            LOG.error("Cannot rename partition directory from " + srcPath + " to " + destPath, e);
            throw new InvalidOperationException("Unable to access src or dest location for partition "
                + tbl.getDbName() + "." + tbl.getTableName() + " " + new_part.getValues());
-          } catch (MetaException me) {
+          } catch (MetaException | NoSuchObjectException me) {


Wrapped MetaException with MetaException.

sankarh · 2018-05-15T16:38:09Z

itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java

+    LOG.info("Testing " + name);
+    String dbName = name + "_" + tid;
+    run("CREATE DATABASE " + dbName + " WITH DBPROPERTIES ( '" +
+            SOURCE_OF_REPLICATION + "' = '1,2,3')", myDriver);


Does this value '1,2,3' have any meaning? If not, can we set it to "true" or "false"?

as per the design ..it should have the replication ids

sankarh · 2018-05-15T16:46:05Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java

@@ -1031,7 +1031,7 @@ public void create_catalog(CreateCatalogRequest rqst)
          if (!success) {
            ms.rollbackTransaction();
            if (madeDir) {
-              wh.deleteDir(catPath, true);
+              wh.deleteDir(catPath, true, false, true);


No data in catalog path.

sankarh · 2018-05-15T16:47:05Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java

@@ -1381,6 +1381,10 @@ private void drop_database_core(RawStore ms, String catName,
        ms.openTransaction();
        db = ms.getDatabase(catName, name);

+        if (!isInTest && ReplChangeManager.isReplPolicySet(db)) {
+          throw new InvalidOperationException("can not drop a database which is a source of replication");


Why is this limitation?

if a database is source of replication then can not drop it. thats the requirement.

sankarh · 2018-05-15T16:56:17Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java

@@ -1161,7 +1161,7 @@ private void dropCatalogCore(String catName)
        success = ms.commitTransaction();
      } finally {
        if (success) {
-          wh.deleteDir(wh.getDnsPath(new Path(cat.getLocationUri())), false);
+          wh.deleteDir(wh.getDnsPath(new Path(cat.getLocationUri())), false, false, true);


Catalog path won't have any data and hence need not recycle.

sankarh · 2018-05-15T17:06:21Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+    return replPolicyIds != null && !replPolicyIds.isEmpty();
+  }
+
+  public static String getReplPolicyIdString(Database db) {


Need not be public method as it is called within this class.

as its storing ids ..its better to have a public function to get the ids ..if its required in future

move the repl id extract logic to database object

database is an auto generated file so it is not moved there

sankarh · 2018-05-15T17:09:39Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

@@ -467,4 +470,27 @@ static void scheduleCMClearer(Configuration conf) {
          0, MetastoreConf.getTimeVar(conf, ConfVars.REPLCMINTERVAL, TimeUnit.SECONDS), TimeUnit.SECONDS);
    }
  }
+
+  public static boolean isReplPolicySet(Database db) {


Can have uniform naming like isSourceOfRepl?

sankarh · 2018-05-15T17:10:48Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

@@ -467,4 +470,27 @@ static void scheduleCMClearer(Configuration conf) {
          0, MetastoreConf.getTimeVar(conf, ConfVars.REPLCMINTERVAL, TimeUnit.SECONDS), TimeUnit.SECONDS);
    }
  }
+
+  public static boolean isReplPolicySet(Database db) {


Can we move this isReplPolicySet method as public method in Database object itself? That may be clean.

i think ..its related to repl change manager ..so better to keep it here ..it will be easier to track if any changes are done

sankarh · 2018-05-15T17:11:52Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java

@@ -4024,6 +4022,7 @@ private boolean drop_partition_common(RawStore ms, String catName, String db_nam
                                                      new DropPartitionEvent(tbl, part, true, deleteData, this),
                                                      envContext);
          }
+          isReplEnabled = ReplChangeManager.isReplPolicySet(ms.getDatabase(catName, db_name));


isReplEnabled name is misleading. Can use isNeedCmRecycle itself.

the function is giving if repl is enabled or not ...or if its a source of replication ..recycle or not ..thats is different functionality

sankarh · 2018-05-21T09:35:14Z

itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java

+  public void testDumpNonReplDatabase() throws IOException {
+    String dbName = createDBNonRepl(testName.getMethodName(), driver);
+    verifyFail("REPL DUMP " + dbName, driver);
+    verifyFail("REPL DUMP " + dbName + " from 1 ", driver);


Shall Alter database to add source_of_replication DB property and see if Dump succeed afterwards.

sankarh · 2018-05-21T09:48:19Z

ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java

+        try {
+          initReplDump(ast);
+        } catch (HiveException e) {
+          throw new SemanticException("repl dump failed " + e.getMessage());


Why need to convert HiveException to SemanticException. Instead initReplDump shall throw SemanticException itself right?

getDatabase will throw hiveExcpetion ..either need to convert it at initreplDump or here

Converting in initReplDump seems cleaner. Also, check if it harms if we don't convert as SemanticException is derived from HiveException only.

sankarh · 2018-05-21T09:48:49Z

ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java

    int numChildren = ast.getChildCount();
    dbNameOrPattern = PlanUtils.stripQuotes(ast.getChild(0).getText());
+
+    for (String dbName : Utils.matchesDb(db, dbNameOrPattern)) {
+      if (dbName.equalsIgnoreCase(Warehouse.DEFAULT_DATABASE_NAME)) {


Why default database exempted from source_of_replication check? Also, CM never recycle any dropped files from default database and so allowing it would be problem.

sankarh · 2018-05-21T09:51:05Z

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java

-      replChangeManager.recycle(dead, ReplChangeManager.RecycleType.MOVE, true);
+      if (ReplChangeManager.isSourceOfReplication(db)) {
+        replChangeManager.recycle(dead, ReplChangeManager.RecycleType.MOVE, true);
+      }
      fs.delete(dead, true);


If CM move the file, then why do we need to delete here?

cm may not remove the directory so keeping it as it is

sankarh · 2018-05-21T10:03:52Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java

@@ -633,7 +634,8 @@ public Partition alterPartition(final RawStore msdb, Warehouse wh, final String
              }

              //rename the data directory
-              wh.renameDir(srcPath, destPath, true);
+              wh.renameDir(srcPath, destPath,
+                      ReplChangeManager.isSourceOfReplication(msdb.getDatabase(catName, dbname)));


msdb.getDatabase can be stored earlier as it is used somewhere as well and shall get rid if NoSuchObjectException handling too.

sankarh · 2018-05-21T10:23:06Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+
+  public static boolean isSourceOfReplication(Database db) {
+    // Can not judge, so assuming replication is not enabled.
+    if (db == null) {


Shall just assert db != null.

sankarh · 2018-05-21T10:25:02Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+      return false;
+    }
+    String replPolicyIds = getReplPolicyIdString(db);
+    return replPolicyIds != null && !replPolicyIds.isEmpty();


Shall use !StringUtils.isBlank() which checks for both null and empty.

done ..string util isEmpty checks for null also

sankarh · 2018-05-21T10:28:50Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+  public static String getReplPolicyIdString(Database db) {
+    if (db != null) {
+      Map<String, String> m = db.getParameters();
+      if ((m != null) && (m.containsKey(SOURCE_OF_REPLICATION))) {


Shall check if it works with different case for the key. I mean, set the property name in upper case during create Db or alter db and try if we able to read it here.

sankarh · 2018-05-21T10:29:41Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+        LOG.debug("repl policy for database {} is {}", db.getName(), replPolicyId);
+        return replPolicyId;
+      }
+      LOG.info("Repl policy is not set for database ", db.getName());


This also can be debug log.

sankarh · 2018-05-21T10:31:38Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java

@@ -1761,10 +1767,10 @@ private void create_table_core(final RawStore ms, final Table tbl,

        ms.openTransaction();

-        Database db = ms.getDatabase(tbl.getCatName(), tbl.getDbName());
+        db = ms.getDatabase(tbl.getCatName(), tbl.getDbName());
        if (db == null) {


Won't return null.

sankarh · 2018-05-22T12:57:08Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

@@ -467,4 +470,24 @@ static void scheduleCMClearer(Configuration conf) {
          0, MetastoreConf.getTimeVar(conf, ConfVars.REPLCMINTERVAL, TimeUnit.SECONDS), TimeUnit.SECONDS);
    }
  }
+
+  public static boolean isSourceOfReplication(Database db) {
+    // Can not judge, so assuming replication is not enabled.


This comment is inappropriate here.

sankarh · 2018-05-22T12:57:31Z

standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java

+  }
+
+  public static String getReplPolicyIdString(Database db) {
+    if (db != null) {


db is always != null here.

…s source of replication

…s source of replication : after review comment fix

…s source of replication : after review comment fix2

github-actions · 2020-06-16T16:52:34Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

maheshk114 changed the title ~~HIVE-19488 : enable CM root based on db parameter, identifying a db as source of replication~~ HIVE-19488 : enable CM root based on db parameter, identifying db as a source of replication May 10, 2018

maheshk114 force-pushed the BUG-102289 branch 9 times, most recently from 04549ce to 1bae630 Compare May 14, 2018 05:19

sankarh reviewed May 15, 2018

View reviewed changes

sankarh requested changes May 15, 2018

View reviewed changes

maheshk114 force-pushed the BUG-102289 branch from f4b4419 to 3947d83 Compare May 21, 2018 08:17

sankarh requested changes May 21, 2018

View reviewed changes

maheshk114 force-pushed the BUG-102289 branch from 3947d83 to e212faa Compare May 22, 2018 07:05

sankarh reviewed May 22, 2018

View reviewed changes

maheshk114 force-pushed the BUG-102289 branch from e212faa to ae219df Compare May 23, 2018 05:38

maheshk114 added 3 commits May 27, 2018 12:10

HIVE-19488 : enable CM root based on db parameter, identifying a db a…

7d21e36

…s source of replication

HIVE-19488 : enable CM root based on db parameter, identifying a db a…

430b84a

…s source of replication : after review comment fix

HIVE-19488 : enable CM root based on db parameter, identifying a db a…

7eccdc2

…s source of replication : after review comment fix2

maheshk114 force-pushed the BUG-102289 branch from ae219df to 7eccdc2 Compare May 27, 2018 06:41

github-actions bot added the stale label Jun 16, 2020

github-actions bot closed this Jun 24, 2020

HIVE-19488 : enable CM root based on db parameter, identifying db as a source of replication #345

HIVE-19488 : enable CM root based on db parameter, identifying db as a source of replication #345

Conversation

maheshk114 commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankarh May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankarh May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maheshk114 May 22, 2018 • edited

Choose a reason for hiding this comment

sankarh May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 16, 2020

sankarh May 15, 2018 •

edited

sankarh May 21, 2018 •

edited

maheshk114 May 22, 2018 •

edited

sankarh May 21, 2018 •

edited