You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2024/02/22 20:49:07 UTC
[PR] fixes clearing suspension for offline tables [accumulo]
keith-turner opened a new pull request, #4295:
URL: https://github.com/apache/accumulo/pull/4295
There was code in the manager that seemed to have the intent of clearing suspension markers in tablets for an offline table. This code was not working or tested. This commit fixes this code and adds a tests that validates that suspension markers are removed when a table is taken offline.
fixes #3314
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#discussion_r1499952718
##########
test/src/main/java/org/apache/accumulo/test/manager/SuspendedTabletsIT.java:
##########
@@ -300,27 +255,43 @@ private void suspensionTestBody(TServerKiller serverStopper) throws Exception {
assertEquals(beforeDeathState.hosted.get(server), deadTabletsByServer.get(server));
}
assertEquals(TABLETS, ds.hostedCount + ds.suspendedCount);
- // Restart the first tablet server, making sure it ends up on the same port
- HostAndPort restartedServer = deadTabletsByServer.keySet().iterator().next();
- log.info("Restarting " + restartedServer);
- getCluster().getClusterControl().start(ServerType.TABLET_SERVER,
- Map.of(Property.TSERV_CLIENTPORT.getKey(), "" + restartedServer.getPort(),
- Property.TSERV_PORTSEARCH.getKey(), "false"),
- 1);
-
- // Eventually, the suspended tablets should be reassigned to the newly alive tserver.
- log.info("Awaiting tablet unsuspension for tablets belonging to " + restartedServer);
- while (ds.suspended.containsKey(restartedServer) || ds.assignedCount != 0) {
- Thread.sleep(1000);
- ds = TabletLocations.retrieve(ctx, tableName);
- }
- assertEquals(deadTabletsByServer.get(restartedServer), ds.hosted.get(restartedServer));
- // Finally, after much longer, remaining suspended tablets should be reassigned.
- log.info("Awaiting tablet reassignment for remaining tablets");
- while (ds.hostedCount != TABLETS) {
- Thread.sleep(1000);
- ds = TabletLocations.retrieve(ctx, tableName);
+ assertTrue(ds.suspendedCount > 0);
+
+ if (action == AfterSuspendAction.OFFLINE) {
+ client.tableOperations().offline(tableName, true);
+
+ while (ds.suspendedCount > 0) {
+ Thread.sleep(1000);
+ ds = TabletLocations.retrieve(ctx, tableName);
+ log.info("Waiting for suspended {}", ds.suspended);
+ }
+ } else if (action == AfterSuspendAction.RESUME) {
Review Comment:
The code in this else block was not changed except for indentation and being placed in an else block.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#issuecomment-1960338353
> In both of those cases when the tablet(s) are reloaded, is the suspend state still set and subject to the timeout?
Yeah I think so and the existing test was checking for this. I added the following line to the test after tservers were killed to be sure that tablets were in the suspended state.
```
assertTrue(ds.suspendedCount > 0);
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#discussion_r1499953770
##########
test/src/main/java/org/apache/accumulo/test/manager/SuspendedTabletsIT.java:
##########
@@ -330,6 +301,95 @@ void eliminateTabletServers(ClientContext ctx, TabletLocations locs, int count)
throws Exception;
}
+ private class ShutdownTserverKiller implements TServerKiller {
+
+ @Override
+ public void eliminateTabletServers(ClientContext ctx, TabletLocations locs, int count)
Review Comment:
The code in the method was copied w/o change from an existing lambda in the test. This copy was done for reuse.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#issuecomment-1960331467
With these changes. What is the suspend state when:
1. A tablet server is stopped.
2. A tablet server crashes (killed)
In both of those cases when the tablet(s) are reloaded, is the suspend state still set and subject to the timeout?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#discussion_r1499952718
##########
test/src/main/java/org/apache/accumulo/test/manager/SuspendedTabletsIT.java:
##########
@@ -300,27 +255,43 @@ private void suspensionTestBody(TServerKiller serverStopper) throws Exception {
assertEquals(beforeDeathState.hosted.get(server), deadTabletsByServer.get(server));
}
assertEquals(TABLETS, ds.hostedCount + ds.suspendedCount);
- // Restart the first tablet server, making sure it ends up on the same port
- HostAndPort restartedServer = deadTabletsByServer.keySet().iterator().next();
- log.info("Restarting " + restartedServer);
- getCluster().getClusterControl().start(ServerType.TABLET_SERVER,
- Map.of(Property.TSERV_CLIENTPORT.getKey(), "" + restartedServer.getPort(),
- Property.TSERV_PORTSEARCH.getKey(), "false"),
- 1);
-
- // Eventually, the suspended tablets should be reassigned to the newly alive tserver.
- log.info("Awaiting tablet unsuspension for tablets belonging to " + restartedServer);
- while (ds.suspended.containsKey(restartedServer) || ds.assignedCount != 0) {
- Thread.sleep(1000);
- ds = TabletLocations.retrieve(ctx, tableName);
- }
- assertEquals(deadTabletsByServer.get(restartedServer), ds.hosted.get(restartedServer));
- // Finally, after much longer, remaining suspended tablets should be reassigned.
- log.info("Awaiting tablet reassignment for remaining tablets");
- while (ds.hostedCount != TABLETS) {
- Thread.sleep(1000);
- ds = TabletLocations.retrieve(ctx, tableName);
+ assertTrue(ds.suspendedCount > 0);
+
+ if (action == AfterSuspendAction.OFFLINE) {
+ client.tableOperations().offline(tableName, true);
+
+ while (ds.suspendedCount > 0) {
+ Thread.sleep(1000);
+ ds = TabletLocations.retrieve(ctx, tableName);
+ log.info("Waiting for suspended {}", ds.suspended);
+ }
+ } else if (action == AfterSuspendAction.RESUME) {
Review Comment:
The code in this else block was not changed except for indentation.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4295:
URL: https://github.com/apache/accumulo/pull/4295
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#issuecomment-1960513598
> As long as this preserves the behavior that the suspend duration can be used for rolling restarts with tservers either stopped by command or by a kill (crash).
I did not change that behavior in this PR.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes clearing suspension for offline tables [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4295:
URL: https://github.com/apache/accumulo/pull/4295#discussion_r1499954148
##########
test/src/main/java/org/apache/accumulo/test/manager/SuspendedTabletsIT.java:
##########
@@ -330,6 +301,95 @@ void eliminateTabletServers(ClientContext ctx, TabletLocations locs, int count)
throws Exception;
}
+ private class ShutdownTserverKiller implements TServerKiller {
+
+ @Override
+ public void eliminateTabletServers(ClientContext ctx, TabletLocations locs, int count)
+ throws Exception {
+
+ Set<TServerInstance> tserverSet = new HashSet<>();
+ Set<TServerInstance> metadataServerSet = new HashSet<>();
+
+ TabletLocator tl = TabletLocator.getLocator(ctx, MetadataTable.ID);
+ for (TabletLocationState tls : locs.locationStates.values()) {
+ if (tls.current != null) {
+ // add to set of all servers
+ tserverSet.add(tls.current.getServerInstance());
+
+ // get server that the current tablets metadata is on
+ TabletLocator.TabletLocation tab =
+ tl.locateTablet(ctx, tls.extent.toMetaRow(), false, false);
+ // add it to the set of servers with metadata
+ metadataServerSet
+ .add(new TServerInstance(tab.tablet_location, Long.valueOf(tab.tablet_session, 16)));
+ }
+ }
+
+ // remove servers with metadata on them from the list of servers to be shutdown
+ assertEquals(1, metadataServerSet.size(), "Expecting a single tServer in metadataServerSet");
+ tserverSet.removeAll(metadataServerSet);
+
+ assertEquals(TSERVERS - 1, tserverSet.size(),
+ "Expecting " + (TSERVERS - 1) + " tServers in shutdown-list");
+
+ List<TServerInstance> tserversList = new ArrayList<>(tserverSet);
+ Collections.shuffle(tserversList, random);
+
+ for (int i1 = 0; i1 < count; ++i1) {
+ final String tserverName = tserversList.get(i1).getHostPortSession();
+ ThriftClientTypes.MANAGER.executeVoid(ctx, client -> {
+ log.info("Sending shutdown command to {} via ManagerClientService", tserverName);
+ client.shutdownTabletServer(null, ctx.rpcCreds(), tserverName, false);
+ });
+ }
+
+ log.info("Waiting for tserver process{} to die", count == 1 ? "" : "es");
+ for (int i2 = 0; i2 < 10; ++i2) {
+ List<ProcessReference> deadProcs = new ArrayList<>();
+ for (ProcessReference pr1 : getCluster().getProcesses().get(ServerType.TABLET_SERVER)) {
+ Process p = pr1.getProcess();
+ if (!p.isAlive()) {
+ deadProcs.add(pr1);
+ }
+ }
+ for (ProcessReference pr2 : deadProcs) {
+ log.info("Process {} is dead, informing cluster control about this", pr2.getProcess());
+ getCluster().getClusterControl().killProcess(ServerType.TABLET_SERVER, pr2);
+ --count;
+ }
+ if (count == 0) {
+ return;
+ } else {
+ Thread.sleep(SECONDS.toMillis(2));
+ }
+ }
+ throw new IllegalStateException("Tablet servers didn't die!");
+
+ }
+ }
+
+ private class CrashTserverKiller implements TServerKiller {
+
+ @Override
+ public void eliminateTabletServers(ClientContext ctx, TabletLocations locs, int count)
Review Comment:
The code in this methos was also copied w/o change from an existing lambda in the test
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org