[GitHub] [hive] pkumarsinha commented on a change in pull request #2092: HIVE-24912: Support to add repl.target.for property during incrementa…

[GitBox <gi...@apache.org>]

pkumarsinha commented on a change in pull request #2092:
URL: https://github.com/apache/hive/pull/2092#discussion_r604619364



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/repl/incremental/IncrementalLoadTasksBuilder.java
##########
@@ -163,6 +163,17 @@ public IncrementalLoadTasksBuilder(String dbName, String loadPath,
       taskChainTail.addDependentTask(updateIncPendTask);
       taskChainTail = updateIncPendTask;
 
+      Database dbToLoadIn = hive.getDatabase(dbName);
+      if (dbToLoadIn != null && !ReplUtils.isTargetOfReplication(dbToLoadIn)) {
+        Map<String, String> props = new HashMap<>();
+        props.put(ReplUtils.TARGET_OF_REPLICATION, "true");

Review comment:
       Can you please check if this is not overriding an existing property?

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/repl/incremental/IncrementalLoadTasksBuilder.java
##########
@@ -163,6 +163,17 @@ public IncrementalLoadTasksBuilder(String dbName, String loadPath,
       taskChainTail.addDependentTask(updateIncPendTask);
       taskChainTail = updateIncPendTask;
 
+      Database dbToLoadIn = hive.getDatabase(dbName);
+      if (dbToLoadIn != null && !ReplUtils.isTargetOfReplication(dbToLoadIn)) {

Review comment:
       When would dbToLoadIn be null?

##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
##########
@@ -4287,6 +4288,37 @@ public void testMoveOptimizationIncremental() throws IOException {
     verifyRun("SELECT count(*) from " + replDbName + ".unptned_late ", "3", driverMirror);
   }
 
+  @Test
+  public void testReplTargetSetInIncremental() throws Exception {
+    String name = testName.getMethodName();
+    String dbName = createDB(name, driver);
+    String replDbName = dbName + "_dupe";
+
+    run("CREATE TABLE " + dbName + ".unptned(a string) STORED AS TEXTFILE", driver);
+    Tuple bootstrapDump = bootstrapLoadAndVerify(dbName, replDbName);
+
+    // Remove TARGET_OF_REPLICATION property.
+    run("ALTER DATABASE " + replDbName + " Set DBPROPERTIES ( '" + TARGET_OF_REPLICATION + "' = '')", driverMirror);
+

Review comment:
       Can you verify that the property in target DB indeed doesn't exist.

##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
##########
@@ -4287,6 +4288,37 @@ public void testMoveOptimizationIncremental() throws IOException {
     verifyRun("SELECT count(*) from " + replDbName + ".unptned_late ", "3", driverMirror);
   }
 
+  @Test
+  public void testReplTargetSetInIncremental() throws Exception {
+    String name = testName.getMethodName();
+    String dbName = createDB(name, driver);

Review comment:
       Set some db level property and make sure after incremental the property is present

##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenarios.java
##########
@@ -4287,6 +4288,37 @@ public void testMoveOptimizationIncremental() throws IOException {
     verifyRun("SELECT count(*) from " + replDbName + ".unptned_late ", "3", driverMirror);
   }
 
+  @Test
+  public void testReplTargetSetInIncremental() throws Exception {
+    String name = testName.getMethodName();
+    String dbName = createDB(name, driver);
+    String replDbName = dbName + "_dupe";
+
+    run("CREATE TABLE " + dbName + ".unptned(a string) STORED AS TEXTFILE", driver);
+    Tuple bootstrapDump = bootstrapLoadAndVerify(dbName, replDbName);
+

Review comment:
       Add an assertion for the properties('repl.target.for' as well as extra property you set initially) being present on target db.

##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/TestReplicationScenariosAcrossInstances.java
##########
@@ -1114,13 +1115,18 @@ public void testIfCkptAndSourceOfReplPropsIgnoredByReplDump() throws Throwable {
             .dump(primaryDbName, Collections.emptyList());
 
     // Incremental Repl A -> B with alters on db/table/partition
-    WarehouseInstance.Tuple tupleReplicaInc = replica.load(replicatedDbName, primaryDbName)
-            .run("repl status " + replicatedDbName)
+    replica.load(replicatedDbName, primaryDbName);

Review comment:
       - Perform bootstrap dump and load - should pass
   - Perform dump on target db - should fail
   - Alter target db to remove repl.target.for and perform dump on target db - should pass
   - perform incremental dump and load (src->target). - should pass
   - Perform dump on targetdb  - should fail.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org