keith-turner commented on a change in pull request #1614:
URL: https://github.com/apache/accumulo/pull/1614#discussion_r431953512
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/BulkNewIT.java
##########
@@ -183,6 +186,29 @@ public void testSingleTabletSingleFileOffline() throws
Exception {
}
}
+ @Test
+ public void testMaxTablets() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ tableName = "testMaxTablets_table1";
+ NewTableConfiguration newTableConf = new NewTableConfiguration();
+ // set logical time type so we can set time on bulk import
+ Map<String,String> props = new HashMap<>();
+ props.put(Property.TABLE_BULK_MAX_TABLETS.getKey(), "1");
+ newTableConf.setProperties(props);
+ client.tableOperations().create(tableName, newTableConf);
+
+ // test max tablets hit while inspecting bulk files
+ var thrown = assertThrows(RuntimeException.class, () ->
testBulkFile(false, false));
+ var c = thrown.getCause();
+ assertTrue("Wrong exception: " + c, c instanceof ExecutionException);
+ assertTrue("Wrong exception: " + c.getCause(),
+ c.getCause() instanceof IllegalArgumentException);
+
Review comment:
Not sure if this is possible, but it would be really nice to confirm
that exception message contains the offending file name. Whenever someone runs
into this error message, knowing which file caused the problem will be
extremely helpful to them.
If the test does not do this, I would also recommend creating multiple
files. One that exceeds the limit and few that do not. Want to ensure in this
case the troublesome file is listed in the message.
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/BulkNewIT.java
##########
@@ -183,6 +186,29 @@ public void testSingleTabletSingleFileOffline() throws
Exception {
}
}
+ @Test
+ public void testMaxTablets() throws Exception {
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ tableName = "testMaxTablets_table1";
+ NewTableConfiguration newTableConf = new NewTableConfiguration();
+ // set logical time type so we can set time on bulk import
+ Map<String,String> props = new HashMap<>();
+ props.put(Property.TABLE_BULK_MAX_TABLETS.getKey(), "1");
Review comment:
```suggestion
var props = Map.of(Property.TABLE_BULK_MAX_TABLETS.getKey(), "1");
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -715,6 +715,8 @@
+ " perform specialized parsing of the key. "),
TABLE_BLOOM_HASHTYPE("table.bloom.hash.type", "murmur", PropertyType.STRING,
"The bloom filter hash type"),
+ TABLE_BULK_MAX_TABLETS("table.bulk.max.tablets", "0", PropertyType.COUNT,
+ "The maximum number of tablets allowed for one bulk import file. Value
of 0 is Unlimited"),
Review comment:
Could mention that this property is only enforced when using the new
bulk import API.
----------------------------------------------------------------
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:
[email protected]
