You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/10/22 20:35:57 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1751: WIP Design: Interface for handling tablet state migration
milleruntime opened a new pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751
Some objected oriented design ideas to clean up Master code in ```TabletGroupWatcher```. The
idea is to replace the ```TabletGoalState``` enum with concrete classes, to eliminate some of the complex if/else statements and switch cases. It starts with a Goal interface, implemented by two Abstract types (AbstractGoal + AbstractHostedGoal). The concrete types are classes which combine current state + goal state. The classes are one of two goal categories of HOSTED or not (UNASSIGNED, DELETED, SUSPENDED), since the work is the same for the non hosted goals. I haven't figured out how to integrate the ```MergeState``` that is also used to determine the Goal state [here](https://github.com/apache/accumulo/blob/62110a3c27b9a27d8a7c0bc272aa6778910acad7/server/manager/src/main/java/org/apache/accumulo/master/Master.java#L587).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#issuecomment-715625908
Is the following the code you are trying to simplify?
https://github.com/apache/accumulo/blob/353c611891a8b88b307dcba5e884467d4191b1f2/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java#L252-L343
How much of your goals could accomplished by replacing code blocks w/ functions like below? I think you are also trying to simplify master.getGoalState() and possibly consolidate concerns that currently straddle master.getGoalState() and TabletGroupWatcher? If that is true, then it seems like the following would not accomplish all of your goals.
```java
if (goal == TabletGoalState.HOSTED) {
if (state != TabletState.HOSTED && !tls.walogs.isEmpty()) {
if (master.recoveryManager.recoverLogs(tls.extent, tls.walogs))
continue;
}
switch (state) {
case HOSTED:
if (server.equals(master.migrations.get(tls.extent)))
master.migrations.remove(tls.extent);
break;
case ASSIGNED_TO_DEAD_SERVER:
hostTabletAssignedToDeadServer(...);
break;
case SUSPENDED:
hostSuspendedTablet(...);
break;
case UNASSIGNED:
hostUnassignedTablet(....);
break;
case ASSIGNED:
hostAssignedTablet(...);
break;
}
} else {
switch (state) {
case SUSPENDED:
unassignSuspendedTablet(...);
break;
case UNASSIGNED:
cancelOfflineTableMigrations(tls);
break;
case ASSIGNED_TO_DEAD_SERVER:
unassignAssignedToDeadServerTablet(...);
break;
case HOSTED:
unassignHostedTablet(....);
break;
case ASSIGNED:
break;
}
}
counts[state.ordinal()]++;
}
```
As for the merge code, that code straddles FATE and the Tablet goal state code in a way that is hard to follow. It may need improvement, but I am uncertain what the improvement is.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#discussion_r512121378
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -244,6 +246,7 @@ public void run() {
}
Review comment:
With this design, the "Always follow through with assignments" check would not need to exist, they would have this logic built into the types. For example, ```AssignedTabletHostedGoal``` is the only class that has a current state of ```ASSIGNED```.
I am not sure what this second check is trying to do... something with ordering during shutdown of the whole cluster?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#discussion_r512211089
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/Master.java
##########
@@ -587,8 +586,9 @@ TabletGoalState getTableGoalState(KeyExtent extent) {
TabletGoalState getGoalState(TabletLocationState tls, MergeInfo mergeInfo) {
KeyExtent extent = tls.extent;
// Shutting down?
- TabletGoalState state = getSystemGoalState(tls);
+ TabletGoalState state = getSystemGoalState(extent);
if (state == TabletGoalState.HOSTED) {
+ // TODO shouldn't this be in the upgrade code? this seems very inefficient
Review comment:
I feel like this check should be moved to the upgrade code. Perhaps it was missed in the upgrade code wrangling? Checking on every single tablet assignment seems very inefficient.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#discussion_r512224988
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -230,117 +232,25 @@ public void run() {
sendChopRequest(mergeStats.getMergeInfo(), state, tls);
sendSplitRequest(mergeStats.getMergeInfo(), state, tls);
- // Always follow through with assignments
- if (state == TabletState.ASSIGNED) {
- goal = TabletGoalState.HOSTED;
- }
+ // Always follow through with assignments will give AssignedTabletHostedGoal
// if we are shutting down all the tabletservers, we have to do it in order
- if (goal == TabletGoalState.SUSPENDED && state == TabletState.HOSTED) {
+ // TODO figure out what this is optimization is doing
+ if (goal instanceof HostedTabletNotHostedGoal) {
if (master.serversToShutdown.equals(currentTServers.keySet())) {
if (dependentWatcher != null && dependentWatcher.assignedOrHosted() > 0) {
- goal = TabletGoalState.HOSTED;
+ goal = new HostedTabletHostedGoal(master, tls);
Review comment:
Master.getGoalState() which is called before this, returns a ```goal = TabletGoalState.SUSPENDED``` if the tablet is on the serversToShutdown. Which this check is most likely overriding... if its in the list for this tablet's dependentWatcher? Ugh this code is nearly impossible to follow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#discussion_r512214012
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -230,117 +232,25 @@ public void run() {
sendChopRequest(mergeStats.getMergeInfo(), state, tls);
sendSplitRequest(mergeStats.getMergeInfo(), state, tls);
- // Always follow through with assignments
- if (state == TabletState.ASSIGNED) {
- goal = TabletGoalState.HOSTED;
- }
+ // Always follow through with assignments will give AssignedTabletHostedGoal
// if we are shutting down all the tabletservers, we have to do it in order
- if (goal == TabletGoalState.SUSPENDED && state == TabletState.HOSTED) {
+ // TODO figure out what this is optimization is doing
+ if (goal instanceof HostedTabletNotHostedGoal) {
if (master.serversToShutdown.equals(currentTServers.keySet())) {
if (dependentWatcher != null && dependentWatcher.assignedOrHosted() > 0) {
- goal = TabletGoalState.HOSTED;
+ goal = new HostedTabletHostedGoal(master, tls);
Review comment:
It looks like this optimization is only when shutting down entire cluster but I am not sure what it is doing or why. We remove the goal if the tablet is trying to suspend when shutting down. This is also the only spot that ```dependentWatcher``` is used. I feel like this could be done better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#issuecomment-716664244
> Is the following the code you are trying to simplify?
>
> [TabletGroupWatcher link](https://github.com/apache/accumulo/blob/353c611891a8b88b307dcba5e884467d4191b1f2/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java#L252-L343)
Yes that is where I started.
> How much of your goals could accomplished by replacing code blocks w/ functions like below?
This is certainly an improvement. We could make this change, regardless of any other refactoring I do. It makes this code block easier to read at least.
> I think you are also trying to simplify master.getGoalState() and possibly consolidate concerns that currently straddle master.getGoalState() and TabletGroupWatcher? If that is true, then it seems like the following would not accomplish all of your goals.
Correct.
> As for the merge code, that code straddles FATE and the Tablet goal state code in a way that is hard to follow. It may need improvement, but I am uncertain what the improvement is.
I agree. I was aiming for a design that would allow adding more states to handle this code as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1751: WIP Design: Interface for handling tablet state migration
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1751:
URL: https://github.com/apache/accumulo/pull/1751#discussion_r512121378
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -244,6 +246,7 @@ public void run() {
}
Review comment:
I am not sure what this second check is trying to do... something with ordering during shutdown of the whole cluster?
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -230,117 +232,25 @@ public void run() {
sendChopRequest(mergeStats.getMergeInfo(), state, tls);
sendSplitRequest(mergeStats.getMergeInfo(), state, tls);
- // Always follow through with assignments
- if (state == TabletState.ASSIGNED) {
- goal = TabletGoalState.HOSTED;
- }
+ // Always follow through with assignments will give AssignedTabletHostedGoal
Review comment:
With this design, the "Always follow through with assignments" check would not need to exist, they would have this logic built into the types. For example, ```AssignedTabletHostedGoal``` is the only class that has a current state of ```ASSIGNED```.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org