[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3319: Remove Bulk Import v1 from main

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #3319:
URL: https://github.com/apache/accumulo/pull/3319#discussion_r1172087104


##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -309,22 +309,11 @@ public enum Property {
       "The balancer class that accumulo will use to make tablet assignment and "
           + "migration decisions.",
       "1.3.5"),
-  MANAGER_BULK_RETRIES("manager.bulk.retries", "3", PropertyType.COUNT,
-      "The number of attempts to bulk import a RFile before giving up.", "1.4.0"),
-  MANAGER_BULK_THREADPOOL_SIZE("manager.bulk.threadpool.size", "5", PropertyType.COUNT,
-      "The number of threads to use when coordinating a bulk import.", "1.4.0"),
-  MANAGER_BULK_THREADPOOL_TIMEOUT("manager.bulk.threadpool.timeout", "0s",
-      PropertyType.TIMEDURATION,
-      "The time after which bulk import threads terminate with no work available.  Zero (0) will keep the threads alive indefinitely.",
-      "2.1.0"),
   MANAGER_BULK_TIMEOUT("manager.bulk.timeout", "5m", PropertyType.TIMEDURATION,

Review Comment:
   What about this bulk timeout flag? Do we still use this?



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -309,22 +309,11 @@ public enum Property {
       "The balancer class that accumulo will use to make tablet assignment and "
           + "migration decisions.",
       "1.3.5"),
-  MANAGER_BULK_RETRIES("manager.bulk.retries", "3", PropertyType.COUNT,
-      "The number of attempts to bulk import a RFile before giving up.", "1.4.0"),
-  MANAGER_BULK_THREADPOOL_SIZE("manager.bulk.threadpool.size", "5", PropertyType.COUNT,
-      "The number of threads to use when coordinating a bulk import.", "1.4.0"),
-  MANAGER_BULK_THREADPOOL_TIMEOUT("manager.bulk.threadpool.timeout", "0s",
-      PropertyType.TIMEDURATION,
-      "The time after which bulk import threads terminate with no work available.  Zero (0) will keep the threads alive indefinitely.",
-      "2.1.0"),
   MANAGER_BULK_TIMEOUT("manager.bulk.timeout", "5m", PropertyType.TIMEDURATION,
       "The time to wait for a tablet server to process a bulk import request", "1.4.3"),
   MANAGER_RENAME_THREADS("manager.rename.threadpool.size", "20", PropertyType.COUNT,
       "The number of threads to use when renaming user files during table import or bulk ingest.",
       "2.1.0"),

Review Comment:
   Do we still use these rename threads for the newer bulk ingest? If not, this description could be updated. I'm not sure which is the case.



##########
core/src/main/thrift/client.thrift:
##########
@@ -46,13 +46,13 @@ enum TableOperationExceptionType {
   NOTFOUND
   OFFLINE
   BULK_BAD_INPUT_DIRECTORY
-  BULK_BAD_ERROR_DIRECTORY
+  OBSOLETE_BULK_BAD_ERROR_DIRECTORY

Review Comment:
   If we've already changed the thrift RPCs substantially so that they are already incompatible in other ways, then this rename probably doesn't matter. I'm not sure if that's the case or not.



##########
server/base/src/test/java/org/apache/accumulo/server/conf/ZooBasedConfigurationTest.java:
##########
@@ -167,7 +167,7 @@ public void getPropertiesTest() {
     assertEquals("9998", zbc.get(GC_PORT));
 
     // read a property from the sysconfig
-    assertEquals("3", zbc.get(MANAGER_BULK_RETRIES));
+    assertEquals(5 * 60 * 1000, zbc.getTimeInMillis(MANAGER_BULK_TIMEOUT));

Review Comment:
   For this kind of stuff, it's sometimes helpful to be explicit with something like:
   
   ```suggestion
       assertEquals(MINUTES.toMillis(5), zbc.getTimeInMillis(MANAGER_BULK_TIMEOUT));
   ```



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

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

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