You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2020/02/27 13:51:11 UTC
[GitHub] [mina-sshd] gnodet opened a new pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
gnodet opened a new pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115
…en fail on windows
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein edited a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein edited a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592032903
That would be ideal... if possible, the the relevant timeouts in the tests can simply be written as
```java
private static final Duration TIMEOUT = Duration.ofSeconds(System.getIntProperty("property.controlled.by.profile", ...some default value...)); // or something to this effect.
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein removed a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein removed a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592020367
My concern is that while right now tests are failing fast, in the future there might be some new code we add that causes many tests to fail after waiting a long time - thus increasing the build time even on our laptops significantly from a few minutes to many minutes. This would have a negative impact on the development cycles - and we are both pressed for time since we are doing this in our spare time. Little as it is I would rather spend my time fixing bugs rather than fixing, waiting 40 minutes for the relevant tests to fail and thus signal that I have to go and re-examine the fix...
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet opened a new pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet opened a new pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115
…en fail on windows
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein edited a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein edited a comment on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592036767
I thought of something extra: if indeed we can use a Maven profile then instead of an absolute number for the timeouts we can use a "factor":
```java
// or something to this effect.
private static final Duration TIMEOUT =
Duration.ofSeconds(
(int) (...default value... * System.getFloatProperty("property.controlled.by.profile", 1.0f))));
```
or both (using 2 separate properties) - as the case may be: whether we need an absolute of a relative number
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet merged pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-591987807
I've merged some changes to the CI, but we still have tests that often fail. The goal is to raise the timeout when possible to see if the CI is more green.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592007424
That's right, but CI systems are usually slower than our development
laptops. It does seem to help passing tests however. But I'm not sure to
understand the argument, the problem would only happen while developing and
if some tests are wrong, right ? That should not be the usual case and
test cases usually fail fast for other problems. But you're right, in
theory we could raise the build time _when something is wrong_.
Le jeu. 27 févr. 2020 à 15:53, Lyor Goldstein <no...@github.com> a
écrit :
> The goal is to raise the timeout when possible to see if the CI is more
> green.
>
> Let's be careful not to raise it too much or failing tests will slow the
> tests as a whole - if each test is allowed ~30 sec. to fail and we have
> 100's of tests we might get a total build time of *hours*.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mina-sshd/pull/115?email_source=notifications&email_token=AAAUQNVI6U6A7I3SGFF7RKLRE7HWPA5CNFSM4K433GGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENEUPNQ#issuecomment-592005046>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUQNXDIHJBG5Z4PF3PILTRE7HWPANCNFSM4K433GGA>
> .
>
--
-----------------------
Guillaume Nodet
------------------------
Red Hat, Open Source Integration
Email: gnodet@redhat.com
Web: http://fusesource.com
Blog: http://gnodet.blogspot.com/
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592031325
I understand. I'll investigate if we can use a maven profile specific to
CI to raise the timeouts and keep them lower when running the default `mvn
package` command.
Le jeu. 27 févr. 2020 à 16:30, Lyor Goldstein <no...@github.com> a
écrit :
> My concern is that while right now tests are failing fast, in the future
> there might be some new code we add that causes many tests to fail after
> waiting a long time - thus increasing the build time even on our laptops
> significantly from a few minutes to many minutes. This would have a
> negative impact on the development cycles - and we are both pressed for
> time since we are doing this in our spare time. Little as it is I would
> rather spend my time fixing bugs rather than fixing, waiting 40 minutes for
> the relevant tests to fail and thus signal that I have to go and re-examine
> the fix...
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mina-sshd/pull/115?email_source=notifications&email_token=AAAUQNXF5AU3UMJ5T7KR3ZTRE7L77A5CNFSM4K433GGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENEYK4A#issuecomment-592020848>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUQNSMSZVNJQXTCRNWS4TRE7L77ANCNFSM4K433GGA>
> .
>
--
-----------------------
Guillaume Nodet
------------------------
Red Hat, Open Source Integration
Email: gnodet@redhat.com
Web: http://fusesource.com
Blog: http://gnodet.blogspot.com/
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592036767
I thought of something extra: if indeed we can use a Maven profile then instead of an absolute number for the timeouts we can use a "factor":
```java
// or something to this effect.
private static final Duration TIMEOUT =
Duration.ofSeconds(
(int) (...default value... * System.getFloatProperty("property.controlled.by.profile", 1.0f))));
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet closed pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#discussion_r385159402
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/future/WaitableFuture.java
##########
@@ -65,6 +66,19 @@ default boolean await(long timeout, TimeUnit unit) throws IOException {
return await(unit.toMillis(timeout));
}
+ /**
+ * Wait for the asynchronous operation to complete with the specified timeout.
+ *
+ * @param timeout The maximum duration to wait
+ * @return {@code true} if the operation is completed.
+ * @throws IOException if failed - specifically {@link java.io.InterruptedIOException}
+ * if waiting was interrupted
+ * @see #await(long)
+ */
+ default boolean await(Duration timeout) throws IOException {
+ return await(timeout.toMillis());
Review comment:
In other locations the code interprets `null` as *forever* - let's maintain this convention (or fix the other code - `ClientChannel#waitFor`) - see similar previous comment
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592020848
My concern is that while right now tests are failing fast, in the future there might be some new code we add that causes many tests to fail after waiting a long time - thus increasing the build time even on our laptops significantly from a few minutes to many minutes. This would have a negative impact on the development cycles - and we are both pressed for time since we are doing this in our spare time. Little as it is I would rather spend my time fixing bugs rather than fixing, waiting 40 minutes for the relevant tests to fail and thus signal that I have to go and re-examine the fix...
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#discussion_r385158414
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/future/WaitableFuture.java
##########
@@ -65,6 +66,19 @@ default boolean await(long timeout, TimeUnit unit) throws IOException {
return await(unit.toMillis(timeout));
}
+ /**
+ * Wait for the asynchronous operation to complete with the specified timeout.
+ *
+ * @param timeout The maximum duration to wait
+ * @return {@code true} if the operation is completed.
+ * @throws IOException if failed - specifically {@link java.io.InterruptedIOException}
+ * if waiting was interrupted
+ * @see #await(long)
+ */
+ default boolean await(Duration timeout) throws IOException {
+ return await(timeout.toMillis());
Review comment:
In other locations the code interprets `null` as *forever* - let's maintain this convention (or fix the other code - `ClientChannel#waitFor`)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592005046
>> The goal is to raise the timeout when possible to see if the CI is more green.
Let's be careful not to raise it too much or failing tests will slow the tests as a whole - if each test is allowed ~30 sec. to fail and we have 100's of tests we might get a total build time of **hours**.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592018214
My concern is that while right now tests are failing fast, in the future there might be some new code we add that causes many tests to fail after waiting a long time - thus increasing the build time even on our laptops significantly from a few minutes to many minutes. This would have a negative impact on the development cycles - and we are both pressed for time since we are doing this in our spare time. Little as it is I would rather spend my time fixing bugs rather than fixing, waiting 40 minutes for the relevant tests to fail and thus signal that I have to go and re-examine the fix...
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592004914
Looks fine
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on issue #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#issuecomment-592032903
That would be ideal... if possible, the the relevant timeouts in the tests can simply be written as
```java
private static final Duration TIMEOUT = System.getIntProperty("property.controlled.by.profile", ...some default value...); // or something to this effect.
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #115: Add timeout with Duration and apply larger timeouts to tests that oft…
URL: https://github.com/apache/mina-sshd/pull/115#discussion_r385158167
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/future/VerifiableFuture.java
##########
@@ -57,6 +58,18 @@ default T verify(long timeout, TimeUnit unit) throws IOException {
return verify(unit.toMillis(timeout));
}
+ /**
+ * Wait and verify that the operation was successful
+ *
+ * @param timeout The maximum duration to wait
+ * @return The (same) future instance
+ * @throws IOException If failed to verify successfully on time
+ * @see #verify(long)
+ */
+ default T verify(Duration timeout) throws IOException {
+ return verify(timeout.toMillis());
Review comment:
In other locations the code interprets `null` as *forever* - let's maintain this convention (or fix the other code - `ClientChannel#waitFor`)
```java
default T verify(Duration timeout) throws IOException {
return (duration == null) ? verify() : verify(timeout.toMillis());
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org