[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT

["DomGarguilo (via GitHub)" <gi...@apache.org>]

DomGarguilo commented on code in PR #3318:
URL: https://github.com/apache/accumulo/pull/3318#discussion_r1171348177


##########
test/src/main/java/org/apache/accumulo/test/functional/ManagerAssignmentIT.java:
##########
@@ -146,36 +148,39 @@ public void test() throws Exception {
 
       // set the hosting goal to always
       c.tableOperations().setTabletHostingGoal(tableName, new Range(), TabletHostingGoal.ALWAYS);
-      TabletLocationState always;
-      do {
-        UtilWaitThread.sleep(250);
-        always = getTabletLocationState(c, tableId);
-      } while (always.goal != TabletHostingGoal.ALWAYS && always.current == null);
 
+      Predicate<TabletLocationState> alwaysHostedOrCurrentNotNull =
+          t -> (t.goal == TabletHostingGoal.ALWAYS) || (t.current != null);
+
+      assertTrue(Wait.waitFor(
+          () -> alwaysHostedOrCurrentNotNull.test(getTabletLocationState(c, tableId)), 60000, 250));
+
+      final TabletLocationState always = getTabletLocationState(c, tableId);
+      assertTrue(alwaysHostedOrCurrentNotNull.test(always));
       assertNull(always.future);
       assertEquals(flushed.getCurrentServer(), always.getLastServer());
       assertEquals(TabletHostingGoal.ALWAYS, always.goal);
 
       // set the hosting goal to never
       c.tableOperations().setTabletHostingGoal(tableName, new Range(), TabletHostingGoal.NEVER);
-      TabletLocationState never;
-      do {
-        UtilWaitThread.sleep(250);
-        never = getTabletLocationState(c, tableId);
-      } while (never.goal != TabletHostingGoal.NEVER && never.current != null);
+      Predicate<TabletLocationState> neverHostedOrCurrentNull =
+          t -> (t.goal == TabletHostingGoal.NEVER) || (t.current == null);
+      assertTrue(Wait.waitFor(

Review Comment:
   Yea I was trying to think of a way to streamline things in the case where we want to wait for a condition of a variable and then use that variable later. Right now the wait for method returns a boolean which is then checked in the test to determine whether the condition was satisfied and if the timeout elapsed.
   
   In the suggested method, there is no check if the timeout has elapsed. Another idea could be to return an `Optional` which would be returned in place of `false` in the current implementation, else would contain the value we are testing against. That way we could check the return value to see if the condition was true or not. I'll look into this a bit.



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