keith-turner commented on code in PR #3872:
URL: https://github.com/apache/accumulo/pull/3872#discussion_r1369296589
##########
core/src/main/java/org/apache/accumulo/core/manager/state/TabletManagement.java:
##########
@@ -94,6 +115,10 @@ public TabletMetadata getTabletMetadata() {
return tabletMetadata;
}
+ public String getErrorMessage() {
Review Comment:
```suggestion
/**
* @return exception message is an exception was thrown while computing
this tablets management actions OR null if no exception was seen
*/
public String getErrorMessage() {
```
##########
core/src/main/java/org/apache/accumulo/core/metrics/MetricsProducer.java:
##########
@@ -628,6 +628,27 @@
* <td>Counter</td>
* <td></td>
* </tr>
+ * <tr>
+ * <td>N/A</td>
+ * <td>N/A</td>
+ * <td>{@link #METRICS_MANAGER_ROOT_TGW_ERRORS}</td>
Review Comment:
I really like these new metrics.
##########
core/src/main/java/org/apache/accumulo/core/metrics/MetricsProducer.java:
##########
@@ -674,6 +695,11 @@ public interface MetricsProducer {
String METRICS_GC_POST_OP_DURATION = METRICS_GC_PREFIX + "post.op.duration";
String METRICS_GC_RUN_CYCLE = METRICS_GC_PREFIX + "run.cycle";
+ String METRICS_MANAGER_PREFIX = "accumulo.manager.";
+ String METRICS_MANAGER_ROOT_TGW_ERRORS = METRICS_MANAGER_PREFIX +
"tgw.root.errors";
Review Comment:
Would be better to use something besides `tgw` in the metrics name because
its not something people would understand outside of Accumulo developers.
Could possibly use `tabletmgmt` instead of `tgw` would look
like`accumulo.manager.tabletmgmt.root.errors`.
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/TabletManagementIterator.java:
##########
@@ -429,20 +429,33 @@ protected void consume() throws IOException {
TabletManagement.CONFIGURED_COLUMNS, false, true);
actions.clear();
- if (managerState != ManagerState.NORMAL || current.isEmpty() ||
onlineTables.isEmpty()) {
- // when manager is in the process of starting up or shutting down
return everything.
- actions.add(ManagementAction.NEEDS_LOCATION_UPDATE);
- } else {
- LOG.trace("Evaluating extent: {}", tm);
- computeTabletManagementActions(tm, actions);
+ Exception error = null;
+ try {
+ if (managerState != ManagerState.NORMAL || current.isEmpty() ||
onlineTables.isEmpty()) {
+ // when manager is in the process of starting up or shutting down
return everything.
+ actions.add(ManagementAction.NEEDS_LOCATION_UPDATE);
+ } else {
+ LOG.trace("Evaluating extent: {}", tm);
+ computeTabletManagementActions(tm, actions);
+ }
+ } catch (Exception e) {
+ LOG.error("Error computing tablet management actions for extent: {}",
tm.getExtent(), e);
+ error = e;
}
- if (!actions.isEmpty()) {
- // If we simply returned here, then the client would get the encoded
K,V
- // from the WholeRowIterator. However, it would not know the reason(s)
why
- // it was returned. Insert a K,V pair to represent the reasons. The
client
- // can pull this K,V pair from the results by looking at the colf.
- TabletManagement.addActions(decodedRow, actions);
+ if (!actions.isEmpty() || error != null) {
+ if (error != null) {
+ // Insert the error into K,V pair representing
+ // the tablet metadata.
+ TabletManagement.addError(decodedRow, error);
+ }
+ if (!actions.isEmpty()) {
Review Comment:
Maybe if an exception was seen, the actions should not be trusted and not
populated? There are cases were its probably ok to still populate them, and
others where it may not make sense to populate. So the most cautious approach
seemt to be to no populate.
```suggestion
} else if (!actions.isEmpty()) {
```
--
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]
