keith-turner commented on code in PR #5668:
URL: https://github.com/apache/accumulo/pull/5668#discussion_r2192934471
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/TabletManagementIterator.java:
##########
@@ -232,17 +233,34 @@ public Entry<Key,Value> next() {
};
final Set<ManagementAction> actions = new HashSet<>();
- final TabletMetadata tm =
- TabletMetadata.convertRow(kvIter, TabletManagement.CONFIGURED_COLUMNS,
false, true);
-
Exception error = null;
+ TabletMetadata tm = null;
+ KeyExtent extent = null;
try {
- LOG.trace("Evaluating extent: {}", tm);
- computeTabletManagementActions(tm, actions);
- } catch (Exception e) {
- LOG.error("Error computing tablet management actions for extent: {}",
tm.getExtent(), e);
+ tm = TabletMetadata.convertRow(kvIter,
TabletManagement.CONFIGURED_COLUMNS, false, true);
+ } catch (RuntimeException e) {
+ LOG.error("Failed to convert tablet metadata at row: {}",
Review Comment:
If the type of error were changed from Exception to String then could do the
following here
```java
var row = keys.get(0) == null ? "no key" : keys.get(0).getRow();
LOG.error("Failed to convert tablet metadata at row: {}", row, e);
error = "Failed to convert tablet metadata at row:"+row+"
error:"+e.getMessage();
```
Seems like this would be a small change that would allow more information to
flow back to the manager. All `TabletManagement.addError` does w/ the
exception is call e.getMessage() so could do that in the few places we set
error in this code.
##########
test/src/main/java/org/apache/accumulo/test/functional/TabletManagementIteratorIT.java:
##########
@@ -284,30 +304,109 @@ public void test() throws AccumuloException,
AccumuloSecurityException, TableExi
Map.of(new Path("file:/vol1/accumulo/inst_id"), new
Path("file:/vol2/accumulo/inst_id"));
tabletMgmtParams = createParameters(client, replacements);
expected = Map.of(prevR4, Set.of(NEEDS_VOLUME_REPLACEMENT));
- assertEquals(expected, findTabletsNeedingAttention(client, metaCopy4,
tabletMgmtParams),
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy4, tabletMgmtParams,
false),
"Should have one tablet that needs a volume replacement");
// In preparation for split an offline testing ensure nothing needs
attention
tabletMgmtParams = createParameters(client);
addFiles(client, metaCopy6, t4);
expected = Map.of();
- assertEquals(expected, findTabletsNeedingAttention(client, metaCopy6,
tabletMgmtParams),
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams,
false),
"No tablets should need attention");
// Lower the split threshold for the table, should cause the files added
to need attention.
client.tableOperations().setProperty(tables[3],
Property.TABLE_SPLIT_THRESHOLD.getKey(),
"1K");
expected = Map.of(prevR4, Set.of(NEEDS_SPLITTING));
- assertEquals(expected, findTabletsNeedingAttention(client, metaCopy6,
tabletMgmtParams),
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams,
false),
"Should have one tablet that needs splitting");
// Take the table offline which should prevent the tablet from being
returned for needing to
// split
client.tableOperations().offline(tables[3], false);
tabletMgmtParams = createParameters(client);
expected = Map.of();
- assertEquals(expected, findTabletsNeedingAttention(client, metaCopy6,
tabletMgmtParams),
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams,
false),
"No tablets should need attention");
+ // Bring the table back online to re-confirm that it needs splitting.
Introduce some errors
+ // into the metadata table.
+ client.tableOperations().online(tables[3]);
+ tabletMgmtParams = createParameters(client);
+ expected = Map.of(prevR4, Set.of(NEEDS_SPLITTING));
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams,
false),
+ "Should have one tablet that needs splitting");
+
+ // insert an errant entry into the metadata between tables 1 and 2.
+ TableId badTableId = TableId.of(tableId2.canonical() + "1");
+ try (TabletsMutatorImpl mut = new TabletsMutatorImpl(getServerContext(),
(dl) -> metaCopy6)) {
+ KeyExtent nonExistantTable = new KeyExtent(badTableId, null, null);
+ TabletMutator tm = mut.mutateTablet(nonExistantTable);
+ tm.putLocation(Location.current("fakeServer", "fakeSession"));
+ tm.automaticallyPutServerLock(false);
+ tm.mutate();
+ }
+ IllegalStateException ise = assertThrows(IllegalStateException.class,
+ () -> findTabletsNeedingAttention(client, metaCopy6,
createParameters(client), false));
+ assertEquals("No prev endrow seen. tableId: " + badTableId + " endrow:
null",
+ ise.getMessage());
+
+ // with errors suppressed, should return prior state
+ tabletMgmtParams = createParameters(client);
+ expected = Map.of(prevR4, Set.of(NEEDS_SPLITTING));
+ assertEquals(expected, findTabletsNeedingAttention(client, metaCopy6,
tabletMgmtParams, true),
+ "Should have one tablet that needs splitting");
+
+ // Remove the errant entry
+ try (TabletsMutatorImpl mut = new TabletsMutatorImpl(getServerContext(),
(dl) -> metaCopy6)) {
+ KeyExtent nonExistantTable = new KeyExtent(badTableId, null, null);
+ TabletMutator tm = mut.mutateTablet(nonExistantTable);
+ tm.deleteLocation(Location.current("fakeServer", "fakeSession"));
+ tm.automaticallyPutServerLock(false);
+ tm.mutate();
+ }
+ // should return prior state without error suppression
+ tabletMgmtParams = createParameters(client);
+ expected = Map.of(prevR4, Set.of(NEEDS_SPLITTING));
+ assertEquals(expected,
+ findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams,
false),
+ "Should have one tablet that needs splitting");
+
+ // Remove the metadata constraint from metaCopy6 so we can insert some
bad json
+ client.tableOperations().removeConstraint(metaCopy6, 1);
Review Comment:
May not need to do this because when `metaCopy6` is created it copies the
data from the metadata table but does not seem to copy the config from the
metadata table.
--
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]
