[GitHub] [accumulo] ctubbsii commented on a change in pull request #1892: Converting table creation to pre-split

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

ctubbsii commented on a change in pull request #1892:
URL: https://github.com/apache/accumulo/pull/1892#discussion_r568003641



##########
File path: test/src/main/java/org/apache/accumulo/test/BulkImportMonitoringIT.java
##########
@@ -65,15 +67,21 @@ protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSit
   public void test() throws Exception {
     getCluster().getClusterControl().start(ServerType.MONITOR);
     try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
+
+      // creating table name
       final String tableName = getUniqueNames(1)[0];
-      c.tableOperations().create(tableName);
-      c.tableOperations().setProperty(tableName, Property.TABLE_MAJC_RATIO.getKey(), "1");
-      // splits to slow down bulk import
+      // creating splits
       SortedSet<Text> splits = new TreeSet<>();
       for (int i = 1; i < 0xf; i++) {
         splits.add(new Text(Integer.toHexString(i)));
       }

Review comment:
       @milleruntime I would argue that the functional syntax is designed to be mimic human logical thinking more, so it's not necessarily simpler for new folks learning code. I think the traditional loops are more familiar with developers who have been around longer, but they aren't more intuitive to people who are familiar with functional programming, or new to programming in general. The terminal condition in traditional for loops, in particular, is difficult to grok for new programmers, and prone to off-by-one type errors, even for seasoned developers.
   
   However, I agree that it's not worth it here to replace the whole loop. However, it may be worth it to do something like the following, so the terminal condition of the loop is more obvious:
   
   ```suggestion
         SortedSet<Text> splits = new TreeSet<>();
         IntStream.range(1, 15).forEach(i -> {
           splits.add(new Text(Integer.toHexString(i)));
         });
   ```
   
   The reason it's important to make it more clear what the loop condition is, is because many of our tests depend on a specific number of split points, and understanding the test as a whole. In this case, strangely, there are 14 split points, which would mean that there should be tablets for this table in the end. Based on `IntStream.range()` being `inclusive - exclusive`, this means we can write our loops as `IntStream.range(1, NUM_TABLETS)` and it is easy to understand.




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