keith-turner commented on code in PR #5416:
URL: https://github.com/apache/accumulo/pull/5416#discussion_r2039950284
##########
server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java:
##########
@@ -218,8 +218,6 @@ public LiveTServerSet(ServerContext context, Listener
cback) {
}
public synchronized void startListeningForTabletServerChanges() {
- scanServers();
Review Comment:
why was this removed?
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -530,8 +527,13 @@ ManagerGoalState getManagerGoalState() {
}
public void clearMigrations(TableId tableId) {
- synchronized (migrations) {
- migrations.keySet().removeIf(extent -> extent.tableId().equals(tableId));
+ var ample = getContext().getAmple();
+ // prev row needed for the extent
+ try (var tabletsMetadata = ample.readTablets().forTable(tableId)
+ .fetch(TabletMetadata.ColumnType.PREV_ROW,
TabletMetadata.ColumnType.MIGRATION).build()) {
+ for (TabletMetadata tabletMetadata : tabletsMetadata) {
+
ample.mutateTablet(tabletMetadata.getExtent()).deleteMigration().mutate();
Review Comment:
This method is making an RPC per tablet and could be optimized. However I
think the entire method can be dropped. It is used in two places. It is
called when a table is deleted, this code will delete all tablets columns so do
not need to worry about deleting the migrations. It is called when table
switches to offline, for this case there is no need to be in a hurry to remove
the migrations. The periodic cleanup task will remove them. So I think we can
drop this entire method and remove the two calls to it.
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -880,20 +879,17 @@ private void hostSuspendedTablet(TabletLists tLists,
TabletMetadata tm, Location
private void hostDeadTablet(TabletLists tLists, TabletMetadata tm, Location
location)
throws WalMarkerException {
tLists.assignedToDeadServers.add(tm);
- if
(location.getServerInstance().equals(manager.migrations.get(tm.getExtent()))) {
- manager.migrations.remove(tm.getExtent());
- }
+ manager.conditionallyDeleteMigration(tm.getExtent(),
location.getServerInstance());
TServerInstance tserver = tm.getLocation().getServerInstance();
if (!tLists.logsForDeadServers.containsKey(tserver)) {
tLists.logsForDeadServers.put(tserver,
walStateManager.getWalsInUse(tserver));
}
}
private void cancelOfflineTableMigrations(KeyExtent extent) {
- TServerInstance dest = manager.migrations.get(extent);
TableState tableState =
manager.getTableManager().getTableState(extent.tableId());
- if (dest != null && tableState == TableState.OFFLINE) {
- manager.migrations.remove(extent);
+ if (tableState == TableState.OFFLINE) {
Review Comment:
Could probably drop this entire method and let the migrations be eventually
cleaned up. If we keep the method should avoid doing a single write per
metadata tablet and batch the writes.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -976,31 +966,37 @@ private long balanceTablets() {
tserverStatusForLevel, partitionedMigrations.get(dl), dl,
getTablesForLevel(dl));
wait = Math.max(tabletBalancer.balance(params), wait);
long migrationsOutForLevel = 0;
- for (TabletMigration m :
checkMigrationSanity(statusForBalancerLevel.keySet(),
- params.migrationsOut(), dl)) {
- final KeyExtent ke = KeyExtent.fromTabletId(m.getTablet());
- if (partitionedMigrations.get(dl).contains(ke)) {
- log.warn("balancer requested migration more than once, skipping
{}", m);
- continue;
+ try (
+ var tabletsMutator =
getContext().getAmple().conditionallyMutateTablets(result -> {})) {
+ for (TabletMigration m :
checkMigrationSanity(statusForBalancerLevel.keySet(),
+ params.migrationsOut(), dl)) {
+ final KeyExtent ke = KeyExtent.fromTabletId(m.getTablet());
+ if (partitionedMigrations.get(dl).contains(ke)) {
+ log.warn("balancer requested migration more than once, skipping
{}", m);
+ continue;
+ }
+ migrationsOutForLevel++;
+ tabletsMutator.mutateTablet(ke).requireAbsentOperation()
Review Comment:
Tablets can have a future location, current location, and no location. We
really only want to set migrations when a tablet has a current location.
There could be race conditions where things change while the balance code is
running. We can check the location in the conditional mutation. Could open a
follow on issue for this to add this check.
```suggestion
tabletsMutator.mutateTablet(ke).requireAbsentOperation().requireAnyCurrentLocation()
```
ideally we only want to set the migration location when the current tablet
location differs from the migration location, so could make the check more
strict
```suggestion
tabletsMutator.mutateTablet(ke).requireAbsentOperation().requireCurrentLocationNotEqualTo(m.getNewTabletServer())
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -880,20 +879,17 @@ private void hostSuspendedTablet(TabletLists tLists,
TabletMetadata tm, Location
private void hostDeadTablet(TabletLists tLists, TabletMetadata tm, Location
location)
throws WalMarkerException {
tLists.assignedToDeadServers.add(tm);
- if
(location.getServerInstance().equals(manager.migrations.get(tm.getExtent()))) {
- manager.migrations.remove(tm.getExtent());
- }
+ manager.conditionallyDeleteMigration(tm.getExtent(),
location.getServerInstance());
Review Comment:
This could be removed will comment elsewhere on why.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -677,19 +650,37 @@ public void run() {
* balanceTablets() balances tables by DataLevel. Return the current set of
migrations partitioned
* by DataLevel
*/
- private static Map<DataLevel,Set<KeyExtent>>
- partitionMigrations(final Set<KeyExtent> migrations) {
+ private Map<DataLevel,Set<KeyExtent>> partitionMigrations() {
final Map<DataLevel,Set<KeyExtent>> partitionedMigrations = new
EnumMap<>(DataLevel.class);
// populate to prevent NPE
Review Comment:
```suggestion
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -835,14 +833,15 @@ private void unassignDeadTablet(TabletLists tLists,
TabletMetadata tm) throws Wa
private void hostUnassignedTablet(TabletLists tLists, KeyExtent tablet,
UnassignedTablet unassignedTablet) {
// maybe it's a finishing migration
- TServerInstance dest = manager.migrations.get(tablet);
+ TServerInstance dest =
+ manager.getContext().getAmple().readTablet(tablet,
MIGRATION).getMigration();
if (dest != null) {
// if destination is still good, assign it
if (tLists.destinations.containsKey(dest)) {
tLists.assignments.add(new Assignment(tablet, dest,
unassignedTablet.getLastLocation()));
} else {
// get rid of this migration
- manager.migrations.remove(tablet);
+
manager.getContext().getAmple().mutateTablet(tablet).deleteMigration().mutate();
Review Comment:
This can probably be removed, will comment in more depths elsewhere.
Thinking when we set the future location that would be a natural place to
remove any migrations.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -677,19 +650,37 @@ public void run() {
* balanceTablets() balances tables by DataLevel. Return the current set of
migrations partitioned
* by DataLevel
*/
- private static Map<DataLevel,Set<KeyExtent>>
- partitionMigrations(final Set<KeyExtent> migrations) {
+ private Map<DataLevel,Set<KeyExtent>> partitionMigrations() {
final Map<DataLevel,Set<KeyExtent>> partitionedMigrations = new
EnumMap<>(DataLevel.class);
// populate to prevent NPE
for (DataLevel dl : DataLevel.values()) {
- partitionedMigrations.put(dl, new HashSet<>());
+ Set<KeyExtent> extents = new HashSet<>();
+ // prev row needed for the extent
+ try (var tabletsMetadata =
getContext().getAmple().readTablets().forLevel(dl)
+ .fetch(TabletMetadata.ColumnType.PREV_ROW,
TabletMetadata.ColumnType.MIGRATION).build()) {
+ for (var tabletMetadata : tabletsMetadata) {
Review Comment:
MAde some comments elsewhere about lazily cleaning up migrations for offline
tables. If we do that could filter out migrations related offline tables in
this loop so the balancing code never sees them. This code could also filter
out migrations where the migration location is equal to the tablets current
location, that is just migration that something missed cleaning up and can be
ignored..
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -644,9 +644,7 @@ private TableMgmtStats
manageTablets(Iterator<TabletManagement> iter,
}
switch (state) {
case HOSTED:
- if
(location.getServerInstance().equals(manager.migrations.get(tm.getExtent()))) {
- manager.migrations.remove(tm.getExtent());
- }
+ manager.conditionallyDeleteMigration(tm.getExtent(),
location.getServerInstance());
Review Comment:
I think we can drop this code and instead move what this code was doing to
the migration cleanup code. The code use to remove migrations for the case when
the tablets current location and migration location are equal. We can have the
migration cleanup code look for that case.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -677,19 +650,37 @@ public void run() {
* balanceTablets() balances tables by DataLevel. Return the current set of
migrations partitioned
* by DataLevel
*/
- private static Map<DataLevel,Set<KeyExtent>>
- partitionMigrations(final Set<KeyExtent> migrations) {
+ private Map<DataLevel,Set<KeyExtent>> partitionMigrations() {
final Map<DataLevel,Set<KeyExtent>> partitionedMigrations = new
EnumMap<>(DataLevel.class);
// populate to prevent NPE
for (DataLevel dl : DataLevel.values()) {
- partitionedMigrations.put(dl, new HashSet<>());
+ Set<KeyExtent> extents = new HashSet<>();
+ // prev row needed for the extent
+ try (var tabletsMetadata =
getContext().getAmple().readTablets().forLevel(dl)
+ .fetch(TabletMetadata.ColumnType.PREV_ROW,
TabletMetadata.ColumnType.MIGRATION).build()) {
+ for (var tabletMetadata : tabletsMetadata) {
+ if (tabletMetadata.getMigration() != null) {
+ extents.add(tabletMetadata.getExtent());
+ }
+ }
+ }
+ partitionedMigrations.put(dl, extents);
}
- migrations.forEach(ke -> {
- partitionedMigrations.get(DataLevel.of(ke.tableId())).add(ke);
- });
return partitionedMigrations;
}
+ /**
+ * Delete the migration, if present, for the given extent if the migration
destination is the
+ * provided tserver
+ */
+ void conditionallyDeleteMigration(KeyExtent extent, TServerInstance tserver)
{
Review Comment:
May be able to drop this method if the places that called it are removed.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
