You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2023/04/18 18:53:19 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #3318: Improvements to ManagerAssignmentIT
DomGarguilo opened a new pull request, #3318:
URL: https://github.com/apache/accumulo/pull/3318
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.
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT
Posted by "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`. 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.
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3318:
URL: https://github.com/apache/accumulo/pull/3318#discussion_r1170631769
##########
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:
Not sure if this is workable. I was wondering if this could be shortened with a new waitfor method like the following.
```java
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.
```java
TabletLocationState never = Wait.waitFor(()->getTabletLocationState(c, tableId), t -> (t.goal == TabletHostingGoal.NEVER) || (t.current == null));
```
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3318:
URL: https://github.com/apache/accumulo/pull/3318#discussion_r1171802638
##########
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:
See [comment](https://github.com/apache/accumulo/pull/3292#discussion_r1171795774). I think there is some consolidation that could occur here.
--
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
[GitHub] [accumulo] DomGarguilo merged pull request #3318: Improvements to ManagerAssignmentIT
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #3318:
URL: https://github.com/apache/accumulo/pull/3318
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3318:
URL: https://github.com/apache/accumulo/pull/3318#discussion_r1175289757
##########
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:
If this test looks okay I might merge it in and create a follow on ticket for consolidating/reworking the wait/retry functionality
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3318: Improvements to ManagerAssignmentIT
Posted by "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