HIVE-23539 : Optimize data copy during repl load operation for HDFS b… #1084

pkumarsinha · 2020-06-09T16:47:02Z

…ased staging location

NOTICE

Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HIVE-XXXXX: Fix a typo in YYY)
For more details, please see https://cwiki.apache.org/confluence/display/Hive/HowToContribute

aasha · 2020-06-10T14:23:44Z

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

@@ -367,6 +369,17 @@ public static boolean shouldReplicate(NotificationEvent tableForEvent,
    }
  }

+  public static boolean onSameHDFSFileSystem(Path dataPath, Path tgtPath) {


Unit tests for this method

aasha · 2020-06-11T13:14:52Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

-        context.hiveConf, false, false
-    );
-
+    Path partDataSrc = new Path(event.dataPath() + File.separator + getPartitionName(sourceWarehousePartitionLocation));


Can create a path directly from the constructor of Path instead of using File Separator

Normally yes, but in this case it will not as getPartitionName(sourceWarehousePartitionLocation) returns value with / attached. Path will take that itself as final path.

aasha · 2020-06-11T13:17:00Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

    Task<?> movePartitionTask = null;
    if (loadFileType != LoadFileType.IGNORE) {
      // no need to create move task, if file is moved directly to target location.
-      movePartitionTask = movePartitionTask(table, partSpec, stagingDir, loadFileType);
+      movePartitionTask = movePartitionTask(table, partSpec, moveSource, loadFileType);


why are we moving from source here?

Path moveSource = performOnlyMove ? partDataSrc : stagingDir;
Move src is either partDataSrc(which is repl Staging dir) or load's own local staging dir which is used to do data copy operation.

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

aasha · 2020-06-11T13:25:46Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

-      ptnRootTask = copyTask;
+    if (performOnlyMove) {
+      if (ptnRootTask == null) {
+        ptnRootTask = addPartTask;


will the addPartTask not be added already as part of DDL operation?

No, that happens only when isOnlyDDLOperation is true. It doesn't come down in to this line in that case.

How was addPartTask added before your change?

Not changing anything with respect to addPartTask. Just the copy-move is getting replaced with single move.

aasha · 2020-06-11T13:27:12Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadTable.java

-
-    Task<?> copyTask = ReplCopyTask.getLoadCopyTask(replicationSpec, dataPath, tmpPath, context.hiveConf,
-            false, false);
+    Path moveSrcPath = performOnlyMove ? dataPath : tmpPath;


What is moveSrcPath ? What is tmpPath? dataPath is on source or target? It would be good if we could rename these variables for readability

moveSrcPath is the path used to perform move data to final warehouse dir on target.

aasha · 2020-06-11T13:32:07Z

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

@@ -452,7 +453,7 @@ private static ImportTableDesc getBaseCreateTableDescFromTable(String dbName,
    boolean isAutoPurge = false;
    boolean needRecycle = false;
    boolean copyToMigratedTxnTable = replicationSpec.isMigratingToTxnTable();
-
+    boolean performOnlyMove = replicationSpec.isInReplicationScope() && Utils.onSameHDFSFileSystem(dataPath, tgtPath);


why do we need this extra condition replicationSpec.isInReplicationScope()?

As we would like to use it only in replication cases.

aasha · 2020-06-11T13:35:34Z

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

+          addPartTask.addDependentTask(loadPartTask);
+        }
+        x.getTasks().add(loadPartTask);
+        return addPartTask == null ? loadPartTask : addPartTask;


can be simplified. There is already a not null check above

Yes, but then we will have to have two return statements which I wanted to avoid.

There are already multiple return statements in the same method.

aasha · 2020-06-15T07:07:09Z

.../src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosExclusiveReplica.java

@@ -179,6 +179,63 @@ public void externalTableReplicationWithLocalStaging() throws Throwable {
            .verifyResult("800");
  }

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


Check for empty staging as the data is moved.

aasha · 2020-06-15T07:07:39Z

.../src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosExclusiveReplica.java

@@ -179,6 +179,63 @@ public void externalTableReplicationWithLocalStaging() throws Throwable {
            .verifyResult("800");
  }

+  @Test


Negative test for different staging when the data is not moved.

Comment not clear

aasha · 2020-06-16T13:37:30Z

...e-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcidTables.java

@@ -1539,15 +1539,14 @@ public void testCheckPointingWithSourceTableDataInserted() throws Throwable {
            .run("insert into t2 values (24)")
            .run("insert into t1 values (4)")
            .dump(primaryDbName, dumpClause);
-
+    assertEquals(modifiedTimeTable1CopyFile, fs.listStatus(tablet1Path)[0].getModificationTime());


Why is this changed?

because data is moved now. We are checking this late

aasha · 2020-06-16T13:37:40Z

...e-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcidTables.java

@@ -1596,6 +1595,10 @@ public void testCheckPointingWithNewTablesAdded() throws Throwable {
            .run("insert into t3 values (3)")
            .dump(primaryDbName, dumpClause);

+    assertEquals(modifiedTimeTable1, fs.getFileStatus(tablet1Path).getModificationTime());


Why is this changed?

Same as above

aasha · 2020-06-16T13:42:27Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

-      ptnRootTask = copyTask;
+    if (performOnlyMove) {
+      if (ptnRootTask == null) {
+        ptnRootTask = addPartTask;


How was addPartTask added before your change?

aasha · 2020-06-16T13:43:19Z

ql/src/java/org/apache/hadoop/hive/ql/exec/repl/bootstrap/load/table/LoadPartitions.java

@@ -320,7 +334,7 @@ private void addPartition(boolean hasMorePartitions, AlterTableAddPartitionDesc
  private String getPartitionName(Path partitionMetadataFullPath) {
    //Get partition name by removing the metadata base path.
    //Needed for getting the data path
-    return partitionMetadataFullPath.toString().substring(event.metadataPath().toString().length());
+    return partitionMetadataFullPath.toString().substring(event.metadataPath().toString().length() + 1);


Can we do a split instead at '/'. This may be error prone

Existing method. just changing the index

aasha · 2020-06-16T13:49:59Z

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

    LoadFileType lft;
    boolean isAutoPurge = false;
    boolean needRecycle = false;
    boolean copyToMigratedTxnTable = replicationSpec.isMigratingToTxnTable();
-
+    boolean performOnlyMove = replicationSpec.isInReplicationScope() && Utils.onSameHDFSFileSystem(dataPath, tgtPath);


Can be added as a separate method.

aasha · 2020-06-16T13:50:47Z

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

+          addPartTask.addDependentTask(loadPartTask);
+        }
+        x.getTasks().add(loadPartTask);
+        return addPartTask == null ? loadPartTask : addPartTask;


There are already multiple return statements in the same method.

aasha · 2020-06-16T13:51:48Z

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

+     * If the Repl staging directory ('hive.repl.rootdir') is on the target cluster itself and the FS scheme is hdfs,
+     * data is moved directly from Repl staging data dir of the table to the table's location on target warehouse.
+     */
+    Path moveDataSrc = performOnlyMove ? dataPath : destPath;
    if (replicationSpec.isInReplicationScope() && AcidUtils.isTransactionalTable(table)) {


replicationSpec.isInReplicationScope( is already done here

aasha · 2020-06-16T13:52:30Z

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

+      }
+
+      Task<?> copyTask = null;
+      if (replicationSpec.isInReplicationScope()) {


This check is already done before. Can be simplified

…ased staging location - bootstrap

…ased staging location - bootstrap: fixed tests

…ased staging location - Incremental - Single Move

…ased staging location - Incremental - Single Move - exim

github-actions · 2020-08-24T00:41:04Z

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.

kgyrtkirk added tests pending tests unstable and removed tests pending labels Jun 9, 2020

aasha reviewed Jun 11, 2020

View reviewed changes

pkumarsinha force-pushed the HIVE-23539 branch from 92054da to a701bc0 Compare June 12, 2020 11:57

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 12, 2020

pkumarsinha force-pushed the HIVE-23539 branch from a701bc0 to 5c2fb99 Compare June 15, 2020 14:40

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 15, 2020

pkumarsinha force-pushed the HIVE-23539 branch from 2249716 to ea0c6c8 Compare June 16, 2020 07:59

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 16, 2020

aasha reviewed Jun 16, 2020

View reviewed changes

pkumarsinha force-pushed the HIVE-23539 branch from ea0c6c8 to 215098e Compare June 17, 2020 22:33

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 17, 2020

pkumarsinha force-pushed the HIVE-23539 branch from 215098e to 2f63f13 Compare June 18, 2020 22:30

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 18, 2020

pkumarsinha force-pushed the HIVE-23539 branch from 2f63f13 to 22ee85b Compare June 22, 2020 00:13

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Jun 22, 2020

pkumarsinha force-pushed the HIVE-23539 branch from 22ee85b to 3429e27 Compare June 22, 2020 22:55

kgyrtkirk added tests pending tests failed and removed tests unstable tests pending labels Jun 22, 2020

Pravin Sinha added 4 commits June 24, 2020 04:12

HIVE-23539 : Optimize data copy during repl load operation for HDFS b…

07fee63

…ased staging location - bootstrap

HIVE-23539 : Optimize data copy during repl load operation for HDFS b…

8c7e43d

…ased staging location - bootstrap: fixed tests

HIVE-23539 : Optimize data copy during repl load operation for HDFS b…

cfbeb48

…ased staging location - Incremental - Single Move

HIVE-23539 : Optimize data copy during repl load operation for HDFS b…

a8fb7ec

…ased staging location - Incremental - Single Move - exim

pkumarsinha force-pushed the HIVE-23539 branch from 3429e27 to a8fb7ec Compare June 23, 2020 22:42

kgyrtkirk added tests pending tests unstable and removed tests failed tests pending labels Jun 23, 2020

github-actions bot added the stale label Aug 24, 2020

pkumarsinha closed this Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-23539 : Optimize data copy during repl load operation for HDFS b… #1084

HIVE-23539 : Optimize data copy during repl load operation for HDFS b… #1084

pkumarsinha commented Jun 9, 2020

aasha Jun 10, 2020

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020 •

edited

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020

aasha Jun 16, 2020

pkumarsinha Jun 16, 2020

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020

aasha Jun 11, 2020

pkumarsinha Jun 11, 2020

aasha Jun 16, 2020

aasha Jun 15, 2020

aasha Jun 15, 2020

pkumarsinha Jun 16, 2020

aasha Jun 16, 2020

pkumarsinha Jun 16, 2020

aasha Jun 16, 2020

pkumarsinha Jun 16, 2020

aasha Jun 16, 2020

aasha Jun 16, 2020

pkumarsinha Jun 16, 2020

aasha Jun 16, 2020

aasha Jun 16, 2020

aasha Jun 16, 2020

aasha Jun 16, 2020

github-actions bot commented Aug 24, 2020

HIVE-23539 : Optimize data copy during repl load operation for HDFS b… #1084

HIVE-23539 : Optimize data copy during repl load operation for HDFS b… #1084

Conversation

pkumarsinha commented Jun 9, 2020

NOTICE

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

pkumarsinha Jun 11, 2020 • 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

github-actions bot commented Aug 24, 2020

pkumarsinha Jun 11, 2020 •

edited