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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
