Repository: hive
Updated Branches:
  refs/heads/master be420098f -> 5eed779c6


HIVE-18696: The partition folders might not get cleaned up properly in the 
HiveMetaStore.add_partitions_core method if an exception occurs (Marta Kuczora, 
via Peter Vary)


Project: http://git-wip-us.apache.org/repos/asf/hive/repo
Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/5eed779c
Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/5eed779c
Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/5eed779c

Branch: refs/heads/master
Commit: 5eed779c611c7c766b69f992d76683c58b5772c9
Parents: be42009
Author: Marta Kuczora <kuczo...@cloudera.com>
Authored: Tue Apr 10 14:00:37 2018 +0200
Committer: Peter Vary <pv...@cloudera.com>
Committed: Tue Apr 10 15:42:56 2018 +0200

----------------------------------------------------------------------
 .../hadoop/hive/metastore/HiveMetaStore.java    | 305 ++++++++++---------
 .../metastore/client/TestAddPartitions.java     | 235 +++++++++++---
 .../client/TestAddPartitionsFromPartSpec.java   | 232 +++++++++++---
 3 files changed, 544 insertions(+), 228 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hive/blob/5eed779c/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
----------------------------------------------------------------------
diff --git 
a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 
b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
index 30922ba..c52d337 100644
--- 
a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
+++ 
b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
@@ -3125,54 +3125,15 @@ public class HiveMetaStore extends ThriftHiveMetastore {
       return ret;
     }
 
-    private static class PartValEqWrapper {
-      Partition partition;
-
-      PartValEqWrapper(Partition partition) {
-        this.partition = partition;
-      }
-
-      @Override
-      public int hashCode() {
-        return partition.isSetValues() ? partition.getValues().hashCode() : 0;
-      }
-
-      @Override
-      public boolean equals(Object obj) {
-        if (this == obj) {
-          return true;
-        }
-        if (obj == null || !(obj instanceof PartValEqWrapper)) {
-          return false;
-        }
-        Partition p1 = this.partition, p2 = ((PartValEqWrapper)obj).partition;
-        if (!p1.isSetValues() || !p2.isSetValues()) {
-          return p1.isSetValues() == p2.isSetValues();
-        }
-        if (p1.getValues().size() != p2.getValues().size()) {
-          return false;
-        }
-        for (int i = 0; i < p1.getValues().size(); ++i) {
-          String v1 = p1.getValues().get(i);
-          String v2 = p2.getValues().get(i);
-          if (v1 == null && v2 == null) {
-            continue;
-          }
-          if (v1 == null || !v1.equals(v2)) {
-            return false;
-          }
-        }
-        return true;
-      }
-    }
-
     private static class PartValEqWrapperLite {
       List<String> values;
       String location;
 
       PartValEqWrapperLite(Partition partition) {
         this.values = partition.isSetValues()? partition.getValues() : null;
-        this.location = partition.getSd().getLocation();
+        if (partition.getSd() != null) {
+          this.location = partition.getSd().getLocation();
+        }
       }
 
       @Override
@@ -3220,7 +3181,7 @@ public class HiveMetaStore extends ThriftHiveMetastore {
       logInfo("add_partitions");
       boolean success = false;
       // Ensures that the list doesn't have dups, and keeps track of 
directories we have created.
-      final Map<PartValEqWrapper, Boolean> addedPartitions = new 
ConcurrentHashMap<>();
+      final Map<PartValEqWrapperLite, Boolean> addedPartitions = new 
ConcurrentHashMap<>();
       final List<Partition> newParts = new ArrayList<>();
       final List<Partition> existingParts = new ArrayList<>();
       Table tbl = null;
@@ -3239,78 +3200,103 @@ public class HiveMetaStore extends ThriftHiveMetastore 
{
           firePreEvent(new PreAddPartitionEvent(tbl, parts, this));
         }
 
-        List<Future<Partition>> partFutures = Lists.newArrayList();
-        final Table table = tbl;
+        Set<PartValEqWrapperLite> partsToAdd = new HashSet<>(parts.size());
+        List<Partition> partitionsToAdd = new ArrayList<>(parts.size());
         for (final Partition part : parts) {
+          // Iterate through the partitions and validate them. If one of the 
partitions is
+          // incorrect, an exception will be thrown before the threads which 
create the partition
+          // folders are submitted. This way we can be sure that no partition 
and no partition
+          // folder will be created if the list contains an invalid partition.
           if (!part.getTableName().equals(tblName) || 
!part.getDbName().equals(dbName)) {
-            throw new MetaException("Partition does not belong to target table 
" +
-                getCatalogQualifiedTableName(catName, dbName, tblName) + ": " +
-                    part);
+            String errorMsg = String.format(
+                "Partition does not belong to target table %s. It belongs to 
the table %s.%s : %s",
+                getCatalogQualifiedTableName(catName, dbName, tblName), 
part.getDbName(),
+                part.getTableName(), part.toString());
+            throw new MetaException(errorMsg);
           }
 
           boolean shouldAdd = startAddPartition(ms, part, ifNotExists);
           if (!shouldAdd) {
             existingParts.add(part);
-            LOG.info("Not adding partition " + part + " as it already exists");
+            LOG.info("Not adding partition {} as it already exists", part);
             continue;
           }
 
-          final UserGroupInformation ugi;
-          try {
-            ugi = UserGroupInformation.getCurrentUser();
-          } catch (IOException e) {
-            throw new RuntimeException(e);
-          }
-
-          partFutures.add(threadPool.submit(new Callable<Partition>() {
-            @Override
-            public Partition call() throws Exception {
-              ugi.doAs(new PrivilegedExceptionAction<Object>() {
-                @Override
-                public Object run() throws Exception {
-                  try {
-                    boolean madeDir = createLocationForAddedPartition(table, 
part);
-                    if (addedPartitions.put(new PartValEqWrapper(part), 
madeDir) != null) {
-                      // Technically, for ifNotExists case, we could insert 
one and discard the other
-                      // because the first one now "exists", but it seems 
better to report the problem
-                      // upstream as such a command doesn't make sense.
-                      throw new MetaException("Duplicate partitions in the 
list: " + part);
-                    }
-                    initializeAddedPartition(table, part, madeDir);
-                  } catch (MetaException e) {
-                    throw new IOException(e.getMessage(), e);
-                  }
-                  return null;
-                }
-              });
-              return part;
+          if (!partsToAdd.add(new PartValEqWrapperLite(part))) {
+            // Technically, for ifNotExists case, we could insert one and 
discard the other
+            // because the first one now "exists", but it seems better to 
report the problem
+            // upstream as such a command doesn't make sense.
+            throw new MetaException("Duplicate partitions in the list: " + 
part);
+          }
+
+          partitionsToAdd.add(part);
+        }
+
+        final AtomicBoolean failureOccurred = new AtomicBoolean(false);
+        final Table table = tbl;
+        List<Future<Partition>> partFutures = new 
ArrayList<>(partitionsToAdd.size());
+
+        final UserGroupInformation ugi;
+        try {
+          ugi = UserGroupInformation.getCurrentUser();
+        } catch (IOException e) {
+          throw new RuntimeException(e);
+        }
+
+        for (final Partition partition : partitionsToAdd) {
+          initializePartitionParameters(table, partition);
+
+          partFutures.add(threadPool.submit(() -> {
+            if (failureOccurred.get()) {
+              return null;
             }
+            ugi.doAs((PrivilegedExceptionAction<Partition>) () -> {
+              try {
+                boolean madeDir = createLocationForAddedPartition(table, 
partition);
+                addedPartitions.put(new PartValEqWrapperLite(partition), 
madeDir);
+                initializeAddedPartition(table, partition, madeDir);
+              } catch (MetaException e) {
+                throw new IOException(e.getMessage(), e);
+              }
+              return null;
+            });
+            return partition;
           }));
         }
 
-        try {
-          for (Future<Partition> partFuture : partFutures) {
+        String errorMessage = null;
+        for (Future<Partition> partFuture : partFutures) {
+          try {
             Partition part = partFuture.get();
-            if (part != null) {
+            if (part != null && !failureOccurred.get()) {
               newParts.add(part);
             }
+          } catch (InterruptedException | ExecutionException e) {
+            // If an exception is thrown in the execution of a task, set the 
failureOccurred flag to
+            // true. This flag is visible in the tasks and if its value is 
true, the partition
+            // folders won't be created.
+            // Then iterate through the remaining tasks and wait for them to 
finish. The tasks which
+            // are started before the flag got set will then finish creating 
the partition folders.
+            // The tasks which are started after the flag got set, won't 
create the partition
+            // folders, to avoid unnecessary work.
+            // This way it is sure that all tasks are finished, when entering 
the finally part where
+            // the partition folders are cleaned up. It won't happen that a 
task is still running
+            // when cleaning up the folders, so it is sure we won't have 
leftover folders.
+            // Canceling the other tasks would be also an option but during 
testing it turned out
+            // that it is not a trustworthy solution to avoid leftover folders.
+            failureOccurred.compareAndSet(false, true);
+            errorMessage = e.getMessage();
           }
-        } catch (InterruptedException | ExecutionException e) {
-          // cancel other tasks
-          for (Future<Partition> partFuture : partFutures) {
-            partFuture.cancel(true);
-          }
-          throw new MetaException(e.getMessage());
+        }
+
+        if (failureOccurred.get()) {
+          throw new MetaException(errorMessage);
         }
 
         if (!newParts.isEmpty()) {
-          success = ms.addPartitions(catName, dbName, tblName, newParts);
-        } else {
-          success = true;
+          ms.addPartitions(catName, dbName, tblName, newParts);
         }
 
-        // Setting success to false to make sure that if the listener fails, 
rollback happens.
-        success = false;
         // Notification is generated for newly created partitions only. The 
subset of partitions
         // that already exist (existingParts), will not generate notifications.
         if (!transactionalListeners.isEmpty()) {
@@ -3324,10 +3310,10 @@ public class HiveMetaStore extends ThriftHiveMetastore {
       } finally {
         if (!success) {
           ms.rollbackTransaction();
-          for (Map.Entry<PartValEqWrapper, Boolean> e : 
addedPartitions.entrySet()) {
+          for (Map.Entry<PartValEqWrapperLite, Boolean> e : 
addedPartitions.entrySet()) {
             if (e.getValue()) {
               // we just created this directory - it's not a case of 
pre-creation, so we nuke.
-              wh.deleteDir(new 
Path(e.getKey().partition.getSd().getLocation()), true);
+              wh.deleteDir(new Path(e.getKey().location), true);
             }
           }
 
@@ -3465,70 +3451,98 @@ public class HiveMetaStore extends ThriftHiveMetastore {
         }
 
         firePreEvent(new PreAddPartitionEvent(tbl, partitionSpecProxy, this));
-        List<Future<Partition>> partFutures = Lists.newArrayList();
-        final Table table = tbl;
-        while(partitionIterator.hasNext()) {
+        Set<PartValEqWrapperLite> partsToAdd = new 
HashSet<>(partitionSpecProxy.size());
+        List<Partition> partitionsToAdd = new 
ArrayList<>(partitionSpecProxy.size());
+        while (partitionIterator.hasNext()) {
+          // Iterate through the partitions and validate them. If one of the 
partitions is
+          // incorrect, an exception will be thrown before the threads which 
create the partition
+          // folders are submitted. This way we can be sure that no partition 
or partition folder
+          // will be created if the list contains an invalid partition.
           final Partition part = partitionIterator.getCurrent();
 
           if (!part.getTableName().equalsIgnoreCase(tblName) || 
!part.getDbName().equalsIgnoreCase(dbName)) {
-            throw new MetaException("Partition does not belong to target table 
"
-                + dbName + "." + tblName + ": " + part);
+            String errorMsg = String.format(
+                "Partition does not belong to target table %s.%s. It belongs 
to the table %s.%s : %s",
+                dbName, tblName, part.getDbName(), part.getTableName(), 
part.toString());
+            throw new MetaException(errorMsg);
           }
 
           boolean shouldAdd = startAddPartition(ms, part, ifNotExists);
           if (!shouldAdd) {
-            LOG.info("Not adding partition " + part + " as it already exists");
+            LOG.info("Not adding partition {} as it already exists", part);
             continue;
           }
 
-          final UserGroupInformation ugi;
-          try {
-            ugi = UserGroupInformation.getCurrentUser();
-          } catch (IOException e) {
-            throw new RuntimeException(e);
-          }
-
-          partFutures.add(threadPool.submit(new Callable<Partition>() {
-            @Override public Partition call() throws Exception {
-              ugi.doAs(new PrivilegedExceptionAction<Partition>() {
-                @Override
-                public Partition run() throws Exception {
-                  try {
-                    boolean madeDir = createLocationForAddedPartition(table, 
part);
-                    if (addedPartitions.put(new PartValEqWrapperLite(part), 
madeDir) != null) {
-                      // Technically, for ifNotExists case, we could insert 
one and discard the other
-                      // because the first one now "exists", but it seems 
better to report the problem
-                      // upstream as such a command doesn't make sense.
-                      throw new MetaException("Duplicate partitions in the 
list: " + part);
-                    }
-                    initializeAddedPartition(table, part, madeDir);
-                  } catch (MetaException e) {
-                    throw new IOException(e.getMessage(), e);
-                  }
-                  return null;
-                }
-              });
-              return part;
-            }
-          }));
+          if (!partsToAdd.add(new PartValEqWrapperLite(part))) {
+            // Technically, for ifNotExists case, we could insert one and 
discard the other
+            // because the first one now "exists", but it seems better to 
report the problem
+            // upstream as such a command doesn't make sense.
+            throw new MetaException("Duplicate partitions in the list: " + 
part);
+          }
+
+          partitionsToAdd.add(part);
           partitionIterator.next();
         }
 
+        final AtomicBoolean failureOccurred = new AtomicBoolean(false);
+        List<Future<Partition>> partFutures = new 
ArrayList<>(partitionsToAdd.size());
+        final Table table = tbl;
+
+        final UserGroupInformation ugi;
         try {
-          for (Future<Partition> partFuture : partFutures) {
+          ugi = UserGroupInformation.getCurrentUser();
+        } catch (IOException e) {
+          throw new RuntimeException(e);
+        }
+
+        for (final Partition partition : partitionsToAdd) {
+          initializePartitionParameters(table, partition);
+
+          partFutures.add(threadPool.submit(() -> {
+            if (failureOccurred.get()) {
+              return null;
+            }
+            ugi.doAs((PrivilegedExceptionAction<Partition>) () -> {
+              try {
+                boolean madeDir = createLocationForAddedPartition(table, 
partition);
+                addedPartitions.put(new PartValEqWrapperLite(partition), 
madeDir);
+                initializeAddedPartition(table, partition, madeDir);
+              } catch (MetaException e) {
+                throw new IOException(e.getMessage(), e);
+              }
+              return null;
+            });
+            return partition;
+          }));
+        }
+
+        String errorMessage = null;
+        for (Future<Partition> partFuture : partFutures) {
+          try {
             partFuture.get();
+          } catch (InterruptedException | ExecutionException e) {
+            // If an exception is thrown in the execution of a task, set the 
failureOccurred flag to
+            // true. This flag is visible in the tasks and if its value is 
true, the partition
+            // folders won't be created.
+            // Then iterate through the remaining tasks and wait for them to 
finish. The tasks which
+            // are started before the flag got set will then finish creating 
the partition folders.
+            // The tasks which are started after the flag got set, won't 
create the partition
+            // folders, to avoid unnecessary work.
+            // This way it is sure that all tasks are finished, when entering 
the finally part where
+            // the partition folders are cleaned up. It won't happen that a 
task is still running
+            // when cleaning up the folders, so it is sure we won't have 
leftover folders.
+            // Canceling the other tasks would be also an option but during 
testing it turned out
+            // that it is not a trustworthy solution to avoid leftover folders.
+            failureOccurred.compareAndSet(false, true);
+            errorMessage = e.getMessage();
           }
-        } catch (InterruptedException | ExecutionException e) {
-          // cancel other tasks
-          for (Future<Partition> partFuture : partFutures) {
-            partFuture.cancel(true);
-          }
-          throw new MetaException(e.getMessage());
         }
 
-        success = ms.addPartitions(catName, dbName, tblName, 
partitionSpecProxy, ifNotExists);
-        //setting success to false to make sure that if the listener fails, 
rollback happens.
-        success = false;
+        if (failureOccurred.get()) {
+          throw new MetaException(errorMessage);
+        }
+
+        ms.addPartitions(catName, dbName, tblName, partitionSpecProxy, 
ifNotExists);
 
         if (!transactionalListeners.isEmpty()) {
           transactionalListenerResponses =
@@ -3637,6 +3651,16 @@ public class HiveMetaStore extends ThriftHiveMetastore {
           part.getParameters().get(hive_metastoreConstants.DDL_TIME) == null) {
         part.putToParameters(hive_metastoreConstants.DDL_TIME, 
Long.toString(time));
       }
+    }
+
+    private void initializePartitionParameters(final Table tbl, final 
Partition part)
+        throws MetaException {
+      initializePartitionParameters(tbl,
+          new PartitionSpecProxy.SimplePartitionWrapperIterator(part));
+    }
+
+    private void initializePartitionParameters(final Table tbl,
+        final PartitionSpecProxy.PartitionIterator part) throws MetaException {
 
       // Inherit table properties into partition properties.
       Map<String, String> tblParams = tbl.getParameters();
@@ -3678,6 +3702,7 @@ public class HiveMetaStore extends ThriftHiveMetastore {
         boolean madeDir = createLocationForAddedPartition(tbl, part);
         try {
           initializeAddedPartition(tbl, part, madeDir);
+          initializePartitionParameters(tbl, part);
           success = ms.addPartition(part);
         } finally {
           if (!success && madeDir) {

http://git-wip-us.apache.org/repos/asf/hive/blob/5eed779c/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
----------------------------------------------------------------------
diff --git 
a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
 
b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
index 8555eee..f8497c7 100644
--- 
a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
+++ 
b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
@@ -21,7 +21,9 @@ package org.apache.hadoop.hive.metastore.client;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hive.metastore.IMetaStoreClient;
@@ -434,20 +436,39 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
   @Test(expected = MetaException.class)
   public void testAddPartitionForView() throws Exception {
 
-    Table table = new TableBuilder()
-        .setDbName(DB_NAME)
-        .setTableName(TABLE_NAME)
-        .setType("VIRTUAL_VIEW")
-        .addCol("test_id", "int", "test col id")
-        .addCol("test_value", DEFAULT_COL_TYPE, "test col value")
-        .addPartCol(YEAR_COL_NAME, DEFAULT_COL_TYPE)
-        .setLocation(null)
-        .create(client, metaStore.getConf());
-    Partition partition = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
     client.add_partition(partition);
   }
 
   @Test
+  public void testAddPartitionsForViewNullPartLocation() throws Exception {
+
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
+    partition.getSd().setLocation(null);
+    List<Partition> partitions = Lists.newArrayList(partition);
+    client.add_partitions(partitions);
+    Partition part = client.getPartition(DB_NAME, tableName, "year=2017");
+    Assert.assertNull(part.getSd().getLocation());
+  }
+
+  @Test
+  public void testAddPartitionsForViewNullPartSd() throws Exception {
+
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
+    partition.setSd(null);
+    List<Partition> partitions = Lists.newArrayList(partition);
+    client.add_partitions(partitions);
+    Partition part = client.getPartition(DB_NAME, tableName, "year=2017");
+    Assert.assertNull(part.getSd());
+  }
+
+  @Test
   public void testAddPartitionForExternalTable() throws Exception {
 
     String tableName = "part_add_ext_table";
@@ -736,20 +757,26 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
     client.dropDatabase("parttestdb2", true, true, true);
   }
 
-  @Test(expected = MetaException.class)
+  @Test
   public void testAddPartitionsDuplicateInTheList() throws Exception {
 
     createTable();
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME,
+        Lists.newArrayList("2014", "2015", "2017", "2017", "2018", "2019"));
 
-    Partition partition1 = buildPartition(DB_NAME, TABLE_NAME, "2017");
-    Partition partition2 = buildPartition(DB_NAME, TABLE_NAME, "2016");
-    Partition partition3 = buildPartition(DB_NAME, TABLE_NAME, "2017");
+    try {
+      client.add_partitions(partitions);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+      // Expected exception
+    }
 
-    List<Partition> partitions = new ArrayList<>();
-    partitions.add(partition1);
-    partitions.add(partition2);
-    partitions.add(partition3);
-    client.add_partitions(partitions);
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition partition : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+    }
   }
 
   @Test
@@ -774,22 +801,32 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
     Assert.assertTrue(parts.contains("year=THIS"));
   }
 
-  @Test(expected = AlreadyExistsException.class)
+  @Test
   public void testAddPartitionsAlreadyExists() throws Exception {
 
     createTable();
-    Partition partition = buildPartition(DB_NAME, TABLE_NAME, "2017");
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+    Partition partition =
+        buildPartition(DB_NAME, TABLE_NAME, "2016", tableLocation + 
"/year=2016a");
     client.add_partition(partition);
 
-    Partition partition1 = buildPartition(DB_NAME, TABLE_NAME, "2015");
-    Partition partition2 = buildPartition(DB_NAME, TABLE_NAME, "2017");
-    Partition partition3 = buildPartition(DB_NAME, TABLE_NAME, "2016");
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME,
+        Lists.newArrayList("2014", "2015", "2016", "2017", "2018"));
 
-    List<Partition> partitions = new ArrayList<>();
-    partitions.add(partition1);
-    partitions.add(partition2);
-    partitions.add(partition3);
-    client.add_partitions(partitions);
+    try {
+      client.add_partitions(partitions);
+      Assert.fail("AlreadyExistsException should have happened.");
+    } catch (AlreadyExistsException e) {
+      // Expected exception
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertEquals(1, parts.size());
+    Assert.assertEquals(partition.getValues(), parts.get(0).getValues());
+    for (Partition part : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(part.getSd().getLocation())));
+    }
   }
 
   @Test(expected = MetaException.class)
@@ -877,16 +914,24 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
 
     createTable();
     String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
-    Partition partition1 = buildPartition(DB_NAME, TABLE_NAME, "2016", 
tableLocation + "/year=2016");
-    Partition partition2 = buildPartition(DB_NAME, TABLE_NAME, "2017", 
tableLocation + "/year=2017");
+    Partition partition1 =
+        buildPartition(DB_NAME, TABLE_NAME, "2016", tableLocation + 
"/year=2016");
+    Partition partition2 =
+        buildPartition(DB_NAME, TABLE_NAME, "2017", tableLocation + 
"/year=2017");
     Partition partition3 =
         buildPartition(Lists.newArrayList("2015", "march"), 
getYearAndMonthPartCols(), 1);
     partition3.getSd().setLocation(tableLocation + "/year=2015/month=march");
+    Partition partition4 =
+        buildPartition(DB_NAME, TABLE_NAME, "2018", tableLocation + 
"/year=2018");
+    Partition partition5 =
+        buildPartition(DB_NAME, TABLE_NAME, "2019", tableLocation + 
"/year=2019");
 
     List<Partition> partitions = new ArrayList<>();
     partitions.add(partition1);
     partitions.add(partition2);
     partitions.add(partition3);
+    partitions.add(partition4);
+    partitions.add(partition5);
 
     try {
       client.add_partitions(partitions);
@@ -898,15 +943,9 @@ public class TestAddPartitions extends MetaStoreClientTest 
{
     List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
     Assert.assertNotNull(parts);
     Assert.assertTrue(parts.isEmpty());
-    // TODO: This does not work correctly. None of the partitions is created, 
but the folder
-    // for the first two is created. It is because in 
HiveMetaStore.add_partitions_core when
-    // going through the partitions, the first two are already put and started 
in the thread
-    // pool when the exception occurs in the third one. When the exception 
occurs, we go to
-    // the finally part, but the map can be empty (it depends on the progress 
of the other
-    // threads) so the folders won't be deleted.
-//    Assert.assertFalse(metaStore.isPathExists(new Path(tableLocation + 
"/year=2016")));
-//    Assert.assertFalse(metaStore.isPathExists(new Path(tableLocation + 
"/year=2017")));
-    Assert.assertFalse(metaStore.isPathExists(new Path(tableLocation + 
"/year=2015/month=march")));
+    for (Partition part : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(part.getSd().getLocation())));
+    }
   }
 
   @Test(expected = MetaException.class)
@@ -1036,16 +1075,9 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
   @Test(expected=MetaException.class)
   public void testAddPartitionsForView() throws Exception {
 
-    Table table = new TableBuilder()
-        .setDbName(DB_NAME)
-        .setTableName(TABLE_NAME)
-        .setType("VIRTUAL_VIEW")
-        .addCol("test_id", "int", "test col id")
-        .addCol("test_value", "string", "test col value")
-        .addPartCol(YEAR_COL_NAME, DEFAULT_COL_TYPE)
-        .setLocation(null)
-        .create(client, metaStore.getConf());
-    Partition partition = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
     List<Partition> partitions = Lists.newArrayList(partition);
     client.add_partitions(partitions);
   }
@@ -1169,6 +1201,70 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
     Assert.assertEquals("year=__HIVE_DEFAULT_PARTITION__", 
partitionNames.get(0));
   }
 
+  @Test
+  public void testAddPartitionsInvalidLocation() throws Exception {
+
+    createTable();
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+    Map<String, String> valuesAndLocations = new HashMap<>();
+    valuesAndLocations.put("2014", tableLocation + "/year=2014");
+    valuesAndLocations.put("2015", tableLocation + "/year=2015");
+    valuesAndLocations.put("2016", "invalidhost:80000/wrongfolder");
+    valuesAndLocations.put("2017", tableLocation + "/year=2017");
+    valuesAndLocations.put("2018", tableLocation + "/year=2018");
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME, 
valuesAndLocations);
+
+    try {
+      client.add_partitions(partitions);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition partition : partitions) {
+      if 
(!"invalidhost:80000/wrongfolder".equals(partition.getSd().getLocation())) {
+        Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+      }
+    }
+  }
+
+  @Test
+  public void testAddPartitionsMoreThanThreadCountsOneFails() throws Exception 
{
+
+    createTable();
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+
+    List<Partition> partitions = new ArrayList<>();
+    for (int i = 0; i < 50; i++) {
+      String value = String.valueOf(2000 + i);
+      String location = tableLocation + "/year=" + value;
+      if (i == 30) {
+        location = "invalidhost:80000/wrongfolder";
+      }
+      Partition partition = buildPartition(DB_NAME, TABLE_NAME, value, 
location);
+      partitions.add(partition);
+    }
+
+    try {
+      client.add_partitions(partitions);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition partition : partitions) {
+      if 
(!"invalidhost:80000/wrongfolder".equals(partition.getSd().getLocation())) {
+        Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+      }
+    }
+  }
+
   // Tests for List<Partition> add_partitions(List<Partition> partitions,
   // boolean ifNotExists, boolean needResults) method
 
@@ -1531,4 +1627,43 @@ public class TestAddPartitions extends 
MetaStoreClientTest {
     Assert.assertTrue("Per default the skewedInfo column value location map 
should be empty.",
         skewedInfo.getSkewedColValueLocationMaps().isEmpty());
   }
+
+  private List<Partition> buildPartitions(String dbName, String tableName, 
List<String> values)
+      throws MetaException {
+
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + tableName;
+    List<Partition> partitions = new ArrayList<>();
+
+    for (String value : values) {
+      Partition partition =
+          buildPartition(dbName, tableName, value, tableLocation + "/year=" + 
value);
+      partitions.add(partition);
+    }
+    return partitions;
+  }
+
+  private List<Partition> buildPartitions(String dbName, String tableName,
+      Map<String, String> valuesAndLocations) throws MetaException {
+
+    List<Partition> partitions = new ArrayList<>();
+
+    for (Map.Entry<String, String> valueAndLocation : 
valuesAndLocations.entrySet()) {
+      Partition partition =
+          buildPartition(dbName, tableName, valueAndLocation.getKey(), 
valueAndLocation.getValue());
+      partitions.add(partition);
+    }
+    return partitions;
+  }
+
+  private void createView(String tableName) throws Exception {
+    new TableBuilder()
+        .setDbName(DB_NAME)
+        .setTableName(tableName)
+        .setType("VIRTUAL_VIEW")
+        .addCol("test_id", "int", "test col id")
+        .addCol("test_value", "string", "test col value")
+        .addPartCol(YEAR_COL_NAME, DEFAULT_COL_TYPE)
+        .setLocation(null)
+        .create(client, metaStore.getConf());
+  }
 }

http://git-wip-us.apache.org/repos/asf/hive/blob/5eed779c/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
----------------------------------------------------------------------
diff --git 
a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
 
b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
index b32954f..fc0c60f 100644
--- 
a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
+++ 
b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
@@ -519,31 +519,57 @@ public class TestAddPartitionsFromPartSpec extends 
MetaStoreClientTest {
     Assert.assertTrue(metaStore.isPathExists(new 
Path(part.getSd().getLocation())));
   }
 
-  @Test(expected = AlreadyExistsException.class)
+  @Test
   public void testAddPartitionSpecPartAlreadyExists() throws Exception {
 
     createTable();
-    Partition partition = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+    Partition partition =
+        buildPartition(DB_NAME, TABLE_NAME, "2016", tableLocation + 
"/year=2016a");
     client.add_partition(partition);
 
-    Partition newPartition = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME,
+        Lists.newArrayList("2014", "2015", "2016", "2017", "2018"));
     PartitionSpecProxy partitionSpecProxy =
-        buildPartitionSpec(DB_NAME, TABLE_NAME, null, 
Lists.newArrayList(newPartition));
-    client.add_partitions_pspec(partitionSpecProxy);
+        buildPartitionSpec(DB_NAME, TABLE_NAME, null, partitions);
+
+    try {
+      client.add_partitions_pspec(partitionSpecProxy);
+      Assert.fail("AlreadyExistsException should have happened.");
+    } catch (AlreadyExistsException e) {
+      // Expected exception
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertEquals(1, parts.size());
+    Assert.assertEquals(partition.getValues(), parts.get(0).getValues());
+    for (Partition part : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(part.getSd().getLocation())));
+    }
   }
 
-  @Test(expected = MetaException.class)
+  @Test
   public void testAddPartitionSpecPartDuplicateInSpec() throws Exception {
 
     createTable();
-    Partition partition1 = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
-    Partition partition2 = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
-    List<Partition> partitions = new ArrayList<>();
-    partitions.add(partition1);
-    partitions.add(partition2);
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME,
+        Lists.newArrayList("2014", "2015", "2017", "2017", "2018", "2019"));
     PartitionSpecProxy partitionSpecProxy =
         buildPartitionSpec(DB_NAME, TABLE_NAME, null, partitions);
-    client.add_partitions_pspec(partitionSpecProxy);
+    try {
+      client.add_partitions_pspec(partitionSpecProxy);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+      // Expected exception
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition partition : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+    }
   }
 
   @Test(expected = MetaException.class)
@@ -672,20 +698,40 @@ public class TestAddPartitionsFromPartSpec extends 
MetaStoreClientTest {
   @Test(expected=MetaException.class)
   public void testAddPartitionSpecForView() throws Exception {
 
-    Table table = new TableBuilder()
-        .setDbName(DB_NAME)
-        .setTableName(TABLE_NAME)
-        .setType("VIRTUAL_VIEW")
-        .addCol("test_id", "int", "test col id")
-        .addCol("test_value", DEFAULT_COL_TYPE, "test col value")
-        .addPartCol(YEAR_COL_NAME, DEFAULT_COL_TYPE)
-        .setLocation(null)
-        .create(client, metaStore.getConf());
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
+    PartitionSpecProxy partitionSpecProxy =
+        buildPartitionSpec(DB_NAME, tableName, null, 
Lists.newArrayList(partition));
+    client.add_partitions_pspec(partitionSpecProxy);
+  }
 
-    Partition partition = buildPartition(DB_NAME, TABLE_NAME, 
DEFAULT_YEAR_VALUE);
+  @Test
+  public void testAddPartitionSpecForViewNullPartLocation() throws Exception {
+
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
+    partition.getSd().setLocation(null);
     PartitionSpecProxy partitionSpecProxy =
-        buildPartitionSpec(DB_NAME, TABLE_NAME, null, 
Lists.newArrayList(partition));
+        buildPartitionSpec(DB_NAME, tableName, null, 
Lists.newArrayList(partition));
     client.add_partitions_pspec(partitionSpecProxy);
+    Partition part = client.getPartition(DB_NAME, tableName, "year=2017");
+    Assert.assertNull(part.getSd().getLocation());
+  }
+
+  @Test
+  public void testAddPartitionsForViewNullPartSd() throws Exception {
+
+    String tableName = "test_add_partition_view";
+    createView(tableName);
+    Partition partition = buildPartition(DB_NAME, tableName, 
DEFAULT_YEAR_VALUE);
+    partition.setSd(null);
+    PartitionSpecProxy partitionSpecProxy =
+        buildPartitionSpec(DB_NAME, tableName, null, 
Lists.newArrayList(partition));
+    client.add_partitions_pspec(partitionSpecProxy);
+    Partition part = client.getPartition(DB_NAME, tableName, "year=2017");
+    Assert.assertNull(part.getSd());
   }
 
   @Test
@@ -790,33 +836,104 @@ public class TestAddPartitionsFromPartSpec extends 
MetaStoreClientTest {
   public void testAddPartitionSpecOneInvalid() throws Exception {
 
     createTable();
-    Partition partition1 = buildPartition(DB_NAME, TABLE_NAME, "2016");
-    Partition partition2 = buildPartition(DB_NAME, TABLE_NAME, "2017");
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+    Partition partition1 =
+        buildPartition(DB_NAME, TABLE_NAME, "2016", tableLocation + 
"/year=2016");
+    Partition partition2 =
+        buildPartition(DB_NAME, TABLE_NAME, "2017", tableLocation + 
"/year=2017");
     Partition partition3 =
         buildPartition(Lists.newArrayList("2015", "march"), 
getYearAndMonthPartCols(), 1);
-    partition3.getSd().setLocation(metaStore.getWarehouseRoot() + 
"/addparttest");
-    List<Partition> partitions = Lists.newArrayList(partition1, partition2, 
partition3);
+    partition3.getSd().setLocation(tableLocation + "/year=2015/month=march");
+    Partition partition4 =
+        buildPartition(DB_NAME, TABLE_NAME, "2018", tableLocation + 
"/year=2018");
+    Partition partition5 =
+        buildPartition(DB_NAME, TABLE_NAME, "2019", tableLocation + 
"/year=2019");
+    List<Partition> partitions =
+        Lists.newArrayList(partition1, partition2, partition3, partition4, 
partition5);
+    PartitionSpecProxy partitionSpecProxy =
+        buildPartitionSpec(DB_NAME, TABLE_NAME, null, partitions);
+
+    try {
+      client.add_partitions_pspec(partitionSpecProxy);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+      // Expected exception
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition part : partitions) {
+      Assert.assertFalse(metaStore.isPathExists(new 
Path(part.getSd().getLocation())));
+    }
+  }
+
+  @Test
+  public void testAddPartitionSpecInvalidLocation() throws Exception {
+
+    createTable();
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+    Map<String, String> valuesAndLocations = new HashMap<>();
+    valuesAndLocations.put("2014", tableLocation + "/year=2014");
+    valuesAndLocations.put("2015", tableLocation + "/year=2015");
+    valuesAndLocations.put("2016", "invalidhost:80000/wrongfolder");
+    valuesAndLocations.put("2017", tableLocation + "/year=2017");
+    valuesAndLocations.put("2018", tableLocation + "/year=2018");
+    List<Partition> partitions = buildPartitions(DB_NAME, TABLE_NAME, 
valuesAndLocations);
+    PartitionSpecProxy partitionSpecProxy =
+        buildPartitionSpec(DB_NAME, TABLE_NAME, null, partitions);
+
+    try {
+      client.add_partitions_pspec(partitionSpecProxy);
+      Assert.fail("MetaException should have happened.");
+    } catch (MetaException e) {
+      // Expected exception
+    }
+
+    List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
+    Assert.assertNotNull(parts);
+    Assert.assertTrue(parts.isEmpty());
+    for (Partition partition : partitions) {
+      if 
(!"invalidhost:80000/wrongfolder".equals(partition.getSd().getLocation())) {
+        Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+      }
+    }
+  }
+
+  @Test
+  public void testAddPartitionSpecMoreThanThreadCountsOneFails() throws 
Exception {
+
+    createTable();
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + TABLE_NAME;
+
+    List<Partition> partitions = new ArrayList<>();
+    for (int i = 0; i < 50; i++) {
+      String value = String.valueOf(2000 + i);
+      String location = tableLocation + "/year=" + value;
+      if (i == 30) {
+        location = "invalidhost:80000/wrongfolder";
+      }
+      Partition partition = buildPartition(DB_NAME, TABLE_NAME, value, 
location);
+      partitions.add(partition);
+    }
     PartitionSpecProxy partitionSpecProxy =
         buildPartitionSpec(DB_NAME, TABLE_NAME, null, partitions);
+
     try {
       client.add_partitions_pspec(partitionSpecProxy);
-      Assert.fail("MetaException should have occurred.");
+      Assert.fail("MetaException should have happened.");
     } catch (MetaException e) {
-      // This is expected
+      // Expected exception
     }
 
     List<Partition> parts = client.listPartitions(DB_NAME, TABLE_NAME, MAX);
     Assert.assertNotNull(parts);
     Assert.assertTrue(parts.isEmpty());
-    // TODO: This does not work correctly. None of the partitions is created, 
but the folder
-    // for the first two is created. It is because in 
HiveMetaStore.add_partitions_core when
-    // going through the partitions, the first two are already put and started 
in the thread
-    // pool when the exception occurs in the third one.
-    // When the exception occurs, we go to the finally part, but the map can 
be empty
-    // (it depends on the progress of the other threads) so the folders won't 
be deleted.
-    // Assert.assertTrue(metaStore.isPathExists(new 
Path(partition1.getSd().getLocation())));
-    // Assert.assertTrue(metaStore.isPathExists(new 
Path(partition2.getSd().getLocation())));
-    // Assert.assertTrue(metaStore.isPathExists(new 
Path(partition3.getSd().getLocation())));
+    for (Partition partition : partitions) {
+      if 
(!"invalidhost:80000/wrongfolder".equals(partition.getSd().getLocation())) {
+        Assert.assertFalse(metaStore.isPathExists(new 
Path(partition.getSd().getLocation())));
+      }
+    }
   }
 
   // Helper methods
@@ -1049,4 +1166,43 @@ public class TestAddPartitionsFromPartSpec extends 
MetaStoreClientTest {
         sd.getLocation());
     Assert.assertTrue(metaStore.isPathExists(new Path(sd.getLocation())));
   }
+
+  private List<Partition> buildPartitions(String dbName, String tableName, 
List<String> values)
+      throws MetaException {
+
+    String tableLocation = metaStore.getWarehouseRoot() + "/" + tableName;
+    List<Partition> partitions = new ArrayList<>();
+
+    for (String value : values) {
+      Partition partition =
+          buildPartition(dbName, tableName, value, tableLocation + "/year=" + 
value);
+      partitions.add(partition);
+    }
+    return partitions;
+  }
+
+  private List<Partition> buildPartitions(String dbName, String tableName,
+      Map<String, String> valuesAndLocations) throws MetaException {
+
+    List<Partition> partitions = new ArrayList<>();
+
+    for (Map.Entry<String, String> valueAndLocation : 
valuesAndLocations.entrySet()) {
+      Partition partition =
+          buildPartition(dbName, tableName, valueAndLocation.getKey(), 
valueAndLocation.getValue());
+      partitions.add(partition);
+    }
+    return partitions;
+  }
+
+  private void createView(String tableName) throws Exception {
+    new TableBuilder()
+        .setDbName(DB_NAME)
+        .setTableName(tableName)
+        .setType("VIRTUAL_VIEW")
+        .addCol("test_id", "int", "test col id")
+        .addCol("test_value", "string", "test col value")
+        .addPartCol(YEAR_COL_NAME, DEFAULT_COL_TYPE)
+        .setLocation(null)
+        .create(client, metaStore.getConf());
+  }
 }

Reply via email to