HIVE-20517 : Creation of staging directory and Move operation is taking time in S3 #430

maheshk114 · 2018-09-10T05:25:21Z

No description provided.

sankarh · 2018-09-11T11:06:45Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -489,6 +489,11 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
          + "used in conjunction with 'hive.repl.dump.metadata.only' set to false. if 'hive.repl.dump.metadata.only' \n"
          + " is set to true then this config parameter has no effect as external table meta data is flushed \n"
          + " always by default."),
+    REPL_ENABLE_MOVE_OPTIMIZATION("hive.repl.enable.move.optimization", false,


Also, highlight the side effect of setting this flag to true in HDFS system where incremental repl may lead to partial data read.

sankarh · 2018-09-11T11:12:00Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -489,6 +489,11 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
          + "used in conjunction with 'hive.repl.dump.metadata.only' set to false. if 'hive.repl.dump.metadata.only' \n"
          + " is set to true then this config parameter has no effect as external table meta data is flushed \n"
          + " always by default."),
+    REPL_ENABLE_MOVE_OPTIMIZATION("hive.repl.enable.move.optimization", false,
+            "If its set to true, replcopy task copy the source files directly to the destination path. \n"


Shall rephrase the statement as "If its set to true, REPL LOAD copies data files directly to the target table/partition location instead of copying to staging directory first and then move to target location. This optimize the REPL LOAD on object data stores such as S3 or WASB where creating a directory and move files are costly operations."

sankarh · 2018-09-11T11:15:39Z

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

+    run("CREATE TABLE " + tableNamePart + " (fld int) partitioned by (part int) STORED AS TEXTFILE", driver);
+
+    run("insert into " + tableNameNoPart + " values (1) ", driver);
+    run("insert into " + tableNameNoPart + " values (2) ", driver);


Shall just limit to 1 row.

for partitioned table first one goes in add partition flow and second one in insert flow

sankarh · 2018-09-11T11:17:08Z

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

+
+    run("insert into " + tableNameNoPart + " values (1) ", driver);
+    run("insert into " + tableNameNoPart + " values (2) ", driver);
+    verifySetup("SELECT fld from " + tableNameNoPart , new String[]{ "1" , "2" }, driver);


verifySetup can be removed as they are generic scenarios tested by many other test cases.

sankarh · 2018-09-11T11:20:40Z

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

+    verifyRun("SELECT fld from " + tableNamePart + " where part = 10" , new String[]{ "1" , "2"}, driverMirror);
+    verifyRun("SELECT fld from " + tableNamePart + " where part = 11" , new String[]{ "3" }, driverMirror);
+    verifyRun("SELECT fld from " + tableNameNoPart , new String[]{ "1" , "2" }, driverMirror);
+    verifyRun("SELECT count(*) from " + tableNamePart , new String[]{ "3"}, driverMirror);


Shall remove all count(*) queries as they are already verified with values query.

count(*) for check if stats are updated or not

sankarh · 2018-09-11T12:10:16Z

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

@@ -80,6 +84,8 @@
  public static final String FUNCTIONS_ROOT_DIR_NAME = "_functions";
  public static final String CONSTRAINTS_ROOT_DIR_NAME = "_constraints";

+  private static final List<String> CLOUD_SCHEME_PREFIXES = Arrays.asList("s3a", "wasb");


Is it ok to hardcode this?

thats the only way now

sankarh · 2018-09-11T12:14:57Z

ql/src/java/org/apache/hadoop/hive/ql/parse/repl/dump/events/CommitTxnHandler.java

-    withinContext.replicationSpec.setIsReplace(true);
+    // In case of ACID operations, same directory may have many other sub directory for different write id stmt id
+    // combination. So we can not set isreplace to true.
+    withinContext.replicationSpec.setIsReplace(false);


But REPL LOAD copies to sub-directory directly not base dir and so replace flag to true make sense in case of retries.

added a check to remove stale files during copy , if the copy is done from cm path.

So, this change for replace flag can be reverted right?

no ..for acid tables the files need not be replaced

sankarh · 2018-09-11T12:15:41Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ReplCopyWork.java

@@ -49,6 +49,8 @@
  // If set to false, it'll behave as a traditional CopyTask.
  protected boolean readSrcAsFilesList = false;

+  protected boolean deleteDestIfExist = false;


Why is it protected?

it is used by replcopy task

then why do we have public methods to expose this? Can be either private member + public methods or just protected if accessed by extended classes.

changed to private

sankarh · 2018-09-11T12:17:02Z

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

+
+    if (replicationSpec.isInReplicationScope() &&
+            x.getCtx().getConf().getBoolean(REPL_ENABLE_MOVE_OPTIMIZATION.varname, false)) {
+      lft = LoadFileType.IGNORE;


Can't we skip move task instead of IGNORE type?

that can be done only if its a add partition ..for insert operation move is required ..anyways thats handled in a separate patch

sankarh · 2018-09-11T12:17:16Z

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

+      Path destPath;
+      if (replicationSpec.isInReplicationScope() &&
+              x.getCtx().getConf().getBoolean(REPL_ENABLE_MOVE_OPTIMIZATION.varname, false)) {
+        loadFileType = LoadFileType.IGNORE;


Can't we skip move task instead of IGNORE type?

move task is required for statistics

sankarh · 2018-09-15T07:48:57Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -489,6 +489,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
          + "used in conjunction with 'hive.repl.dump.metadata.only' set to false. if 'hive.repl.dump.metadata.only' \n"
          + " is set to true then this config parameter has no effect as external table meta data is flushed \n"
          + " always by default."),
+    REPL_ENABLE_MOVE_OPTIMIZATION("hive.repl.enable.move.optimization", false,
+          "If its set to true, REPL LOAD copies data files directly to the target table/partition location \n"


Shall remove all \n in the message.

some of the parameters are there with \n ?

sankarh · 2018-09-15T07:53:39Z

...t/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcrossInstances.java

@@ -1321,6 +1322,106 @@ public Boolean apply(@Nullable CallerArguments args) {
            .verifyResult(replicatedDbName + ".testFunctionOne");
  }

+  @Test
+  public void testMoveOptimizationBootstrapReplLoadRetryAfterFailureFor() throws Throwable {


Shall remove "For" from test name.

sankarh · 2018-09-15T07:55:07Z

...t/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcrossInstances.java

+    };
+
+    InjectableBehaviourObjectStore.setAddNotificationModifier(callerVerifier);
+    replica.loadFailure(replicadb, tuple.dumpLocation, withConfigs);


Put in try catch block and call InjectableBehaviourObjectStore.resetAddNotificationModifier in finally block.

sankarh · 2018-09-15T08:00:31Z

...t/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcrossInstances.java

+    replica.load(replicadb, tuple.dumpLocation, withConfigs);
+
+    replica.run("use " + replicadb)
+            .run("select country from " + tbl)


Can be select place from tbl where country='india' to we know if the data file is correct.

sankarh · 2018-09-15T08:09:16Z

ql/src/java/org/apache/hadoop/hive/ql/exec/ReplCopyTask.java

    try {
+      Hive hiveDb = Hive.get();


Should we use getHive() of Task class to consider the Task conf?

sankarh · 2018-09-15T08:27:03Z

ql/src/java/org/apache/hadoop/hive/ql/parse/repl/dump/events/CommitTxnHandler.java

-    withinContext.replicationSpec.setIsReplace(true);
+    // In case of ACID operations, same directory may have many other sub directory for different write id stmt id
+    // combination. So we can not set isreplace to true.
+    withinContext.replicationSpec.setIsReplace(false);


So, this change for replace flag can be reverted right?

sankarh · 2018-09-15T08:27:47Z

ql/src/java/org/apache/hadoop/hive/ql/plan/LoadTableDesc.java

-    OVERWRITE_EXISTING
+    OVERWRITE_EXISTING,
+    /**
+     * No need to rename the file, used in case of replication to s3


"No need to move the file".

sankarh · 2018-09-15T08:29:15Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ReplCopyWork.java

@@ -49,6 +49,8 @@
  // If set to false, it'll behave as a traditional CopyTask.
  protected boolean readSrcAsFilesList = false;

+  protected boolean deleteDestIfExist = false;


then why do we have public methods to expose this? Can be either private member + public methods or just protected if accessed by extended classes.

sankarh · 2018-09-15T08:34:36Z

...re-server/src/test/java/org/apache/hadoop/hive/metastore/InjectableBehaviourObjectStore.java

+      Boolean success = addNotificationEventModifier.apply(entry);
+      if ((success != null) && !success) {
+        throw new MetaException("InjectableBehaviourObjectStore: Invalid addNotificationEvent operation on DB: "
+                + entry.getDbName() + " table: " + entry.getTableName() + " event : " + entry.getEventType());


DB and table name need not be set always. shall use String.valueOf(entry.getDbName()) as it return "null" string if null.

printing null is fine i think

sankarh · 2018-09-15T08:36:32Z

...re-server/src/test/java/org/apache/hadoop/hive/metastore/InjectableBehaviourObjectStore.java

+      if ((success != null) && !success) {
+        throw new MetaException("InjectableBehaviourObjectStore: Invalid addNotificationEvent operation on DB: "
+                + entry.getDbName() + " table: " + entry.getTableName() + " event : " + entry.getEventType());
+      }


Need to call super.addNotificationEvent(entry); if modifier method returns true.

…ng time in S3

…ng time in S3 : review comment fix

github-actions · 2020-06-09T09:07:11Z

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 force-pushed the HIVE-20517 branch from 75c506d to b2aa892 Compare September 11, 2018 03:39

sankarh requested changes Sep 11, 2018

View reviewed changes

maheshk114 force-pushed the HIVE-20517 branch 2 times, most recently from a57ce7b to f3cf0bd Compare September 15, 2018 07:21

sankarh requested changes Sep 15, 2018

View reviewed changes

HIVE-20517 : Creation of staging directory and Move operation is taki…

a8b365a

…ng time in S3

maheshk114 force-pushed the HIVE-20517 branch from f3cf0bd to 9bffb7c Compare September 16, 2018 16:36

HIVE-20517 : Creation of staging directory and Move operation is taki…

7af4eab

…ng time in S3 : review comment fix

maheshk114 force-pushed the HIVE-20517 branch from 9bffb7c to 7af4eab Compare September 17, 2018 03:42

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

github-actions bot closed this Jun 16, 2020

HIVE-20517 : Creation of staging directory and Move operation is taking time in S3 #430

HIVE-20517 : Creation of staging directory and Move operation is taking time in S3 #430

Conversation

maheshk114 commented Sep 10, 2018

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

maheshk114 Sep 12, 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

github-actions bot commented Jun 9, 2020

maheshk114 Sep 12, 2018 •

edited