[GitHub] [accumulo] ctubbsii commented on a change in pull request #1888: Fixes #1791 - Flaky test: SuspendedTabletsIT

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1888:
URL: https://github.com/apache/accumulo/pull/1888#discussion_r579404037



##########
File path: test/src/main/java/org/apache/accumulo/test/manager/SuspendedTabletsIT.java
##########
@@ -117,7 +117,12 @@ public void shutdownAndResumeTserver() throws Exception {
     suspensionTestBody((ctx, locs, count) -> {
       Set<TServerInstance> tserversSet = new HashSet<>();
       for (TabletLocationState tls : locs.locationStates.values()) {
-        if (tls.current != null) {
+
+        TabletLocator tl = TabletLocator.getLocator(ctx, MetadataTable.ID);
+        String metadataTserver =
+            tl.locateTablet(ctx, tls.extent.toMetaRow(), false, false).tablet_location;
+        // if the server does not hold the metadata, add it to the list to be shutdown
+        if (tls.current != null && !tls.current.toString().startsWith(metadataTserver)) {

Review comment:
       It looks like this code checks to see if the tablet has a currently assigned location, and *only* if that currently assigned location is *NOT* also hosting the metadata for that tablet's own metadata, then the tserver is okay to shutdown.
   
   However, it seems like there are two problems with this:
   
   1. the tserver could still be added to the list if another tablet adds it, so it could still cause problems, and
   2. this could result in no tservers shutting down, therefore not actually checking the conditions the test is intended to check




----------------------------------------------------------------
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