keith-turner commented on code in PR #4208:
URL: https://github.com/apache/accumulo/pull/4208#discussion_r1475484967
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java:
##########
@@ -172,24 +179,16 @@ public Repo<Manager> call(long tid, Manager manager)
throws Exception {
m = new Mutation(metadataRow);
ServerColumnFamily.DIRECTORY_COLUMN.put(m, new Value(tabletDir));
currentRow = metadataRow;
- sawTabletAvailability = false;
}
- if (TabletColumnFamily.AVAILABILITY_COLUMN.hasColumns(key)) {
- sawTabletAvailability = true;
- }
+ // add the initial tablet availability
+ AVAILABILITY_COLUMN.put(m,
TabletAvailabilityUtil.toValue(initialAvailability));
Review Comment:
This add the tablet availability for every key/value it sees in a row. It
should only add it once for the row.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/ImportConfiguration.java:
##########
@@ -73,6 +80,14 @@ interface Builder {
*/
Builder setKeepMappings(boolean keepMappings);
+ /**
+ * set the initial tablet availability for the table
Review Comment:
Could document the behavior when this function is not called, specify what
will happen in this case.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java:
##########
@@ -123,7 +129,12 @@ public Repo<Manager> call(long tid, Manager manager)
throws Exception {
Text currentRow = null;
int dirCount = 0;
- boolean sawTabletAvailability = false;
+ TabletAvailability initialAvailability =
tableInfo.initialAvailability;
+ if (initialAvailability == null) {
+ log.error("Initial tablet availability is null and shouldn't be,
defaulting to "
+ + TabletAvailability.ONDEMAND);
+ initialAvailability = TabletAvailability.ONDEMAND;
+ }
Review Comment:
It seems like this should only be null if there is a bug in the code, if
that is the case could possibly do the following.
```suggestion
TabletAvailability initialAvailability =
Objects.requireNonnull(tableInfo.initialAvailability);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java:
##########
@@ -155,11 +166,7 @@ public Repo<Manager> call(long tid, Manager manager)
throws Exception {
if (m == null || !currentRow.equals(metadataRow)) {
if (m != null) {
- if (!sawTabletAvailability) {
- // add a default tablet availability
- TabletColumnFamily.AVAILABILITY_COLUMN.put(m,
-
TabletAvailabilityUtil.toValue(TabletAvailability.ONDEMAND));
- }
+ AVAILABILITY_COLUMN.put(m,
TabletAvailabilityUtil.toValue(initialAvailability));
Review Comment:
The code used to track if it saw a tablet availability. Now it always
overwrites the tablet availability and does not consider if the imported
tablets already had availability set. Need to decide what we want in the
following cases.
Case 1 :
1. User exports a table with tablets having different availability values
set.
2. User imports the table and does not explicitly set a desired
availability at import time.
Case 2:
1. User exports a table with tablets having different availability values
set.
2. User imports the table and explicitly sets a desired availability at
import time using the new API in this PR.
Case 3:
1. User exports a table from an older version of Accumulo that does not
have tablet availability.
2. User imports the table and does not explicitly set a desired
availability at import time.
For Case 1 I think it should carry forward the availability values from the
exported table on import. For case 2 I think it should override the the
availability values from the exported table on import using the values
specified in the API. For case 3 it should set ONDEMAND for all imported
tablets because they are not set in the export data or in the 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
