Skip to content

Improvements to ManagerAssignmentIT#3318

Merged
DomGarguilo merged 3 commits intoapache:elasticityfrom
DomGarguilo:mngrAssignmentITimprovements
Apr 24, 2023
Merged

Improvements to ManagerAssignmentIT#3318
DomGarguilo merged 3 commits intoapache:elasticityfrom
DomGarguilo:mngrAssignmentITimprovements

Conversation

@DomGarguilo
Copy link
Member

Fixes #3311

This PR adds the following changes to ManagerAssignmentIT

  • Use Wait.waitFor() blocks in place of while loops
  • rename some variables
  • add some variable to try-with-resources blocks

Note: this PR targets the elasticity branch since this test is only in that branch.

@DomGarguilo DomGarguilo self-assigned this Apr 18, 2023
} while (never.goal != TabletHostingGoal.NEVER && never.current != null);
Predicate<TabletLocationState> neverHostedOrCurrentNull =
t -> (t.goal == TabletHostingGoal.NEVER) || (t.current == null);
assertTrue(Wait.waitFor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is workable. I was wondering if this could be shortened with a new waitfor method like the following.

  public static <T> T waitFor(Supplier<T> supplier, Predicate<T> predicate) {
    var obj = supplier.get();
    while(!predicate.test(obj)) {
      TimeUnit.MILLISECONDS.sleep(10);
      obj = supplier.get();
    }
    return obj;
  }

Then maybe could do something like the following.

TabletLocationState never = Wait.waitFor(()->getTabletLocationState(c, tableId), t -> (t.goal == TabletHostingGoal.NEVER) || (t.current == null));

Copy link
Member Author

@DomGarguilo DomGarguilo Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. An empty optional would be returned in place of false in the current implementation, else it 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment. I think there is some consolidation that could occur here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this test looks okay I might merge it in and create a follow on ticket for consolidating/reworking the wait/retry functionality

# Conflicts:
#	test/src/main/java/org/apache/accumulo/test/functional/ManagerAssignmentIT.java
@DomGarguilo DomGarguilo merged commit 50e3717 into apache:elasticity Apr 24, 2023
@dlmarion dlmarion linked an issue Apr 25, 2023 that may be closed by this pull request
@DomGarguilo DomGarguilo deleted the mngrAssignmentITimprovements branch March 14, 2024 15:24
@ctubbsii ctubbsii added this to the 4.0.0 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

OnDemand Follow-on: ManagerAssignmentIT could use Wait

5 participants