You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/11/27 16:49:25 UTC
[PR] Move Wait utility from test module so it can be used in general code [accumulo]
EdColeman opened a new pull request, #3985:
URL: https://github.com/apache/accumulo/pull/3985
The Wait class, with waitFor() has general utility outside of testing. Moving the class to core.util allows for more general reuse. This change is independent of any additional usages to simplify review.
Other than package and import renaming this PR contains no code changes.
--
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] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3985:
URL: https://github.com/apache/accumulo/pull/3985#issuecomment-1828857857
I moved the sleep method from UtilWaitThread class to the relocated Wait class. In 3.1, the UtilWaitThread class will be empty and can be removed.
The `UtilWaitThread.sleepUninterruptibly` method is no longer needed, we can use the guava version directly (beta status has been removed). In 3.1 the change was made to use the guava version.
--
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] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3985:
URL: https://github.com/apache/accumulo/pull/3985#issuecomment-1828338277
I think it might make sense to consolidate `Wait` and `UtilWaitThread`.
--
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] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3985:
URL: https://github.com/apache/accumulo/pull/3985#discussion_r1407772233
##########
core/src/main/java/org/apache/accumulo/core/util/Wait.java:
##########
@@ -51,6 +56,14 @@ public static int getTimeoutFactor(ToIntFunction<NumberFormatException> onError)
}
+ public static void sleep(long millis) {
+ try {
+ Thread.sleep(millis);
+ } catch (InterruptedException e) {
+ log.error("{}", e.getMessage(), e);
Review Comment:
Should `Thread.currentThread().interrupt();` go 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
Re: [PR] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman closed pull request #3985: Move Wait utility from test module so it can be used in general code
URL: https://github.com/apache/accumulo/pull/3985
--
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] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3985:
URL: https://github.com/apache/accumulo/pull/3985#discussion_r1407820438
##########
core/src/main/java/org/apache/accumulo/core/util/Wait.java:
##########
@@ -51,6 +56,14 @@ public static int getTimeoutFactor(ToIntFunction<NumberFormatException> onError)
}
+ public static void sleep(long millis) {
+ try {
+ Thread.sleep(millis);
+ } catch (InterruptedException e) {
+ log.error("{}", e.getMessage(), e);
Review Comment:
Probably, but it was not in the original code - and I don't think anyone is actually checking the flag status anyway. It would be better if we did not use this method and examined interrupt handling in general, but that is beyond the scope of 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] Move Wait utility from test module so it can be used in general code [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3985:
URL: https://github.com/apache/accumulo/pull/3985#issuecomment-1832457713
I'm going to try another approach that will remove the timeout factor from the common wait code at will be submitted as a new 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