You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/03/13 18:31:27 UTC
[GitHub] [cassandra] yifan-c opened a new pull request #470: fix
testSerializeError
yifan-c opened a new pull request #470: fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393923231
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -240,6 +247,27 @@ private void doCheck(FailCheck testAndFailCheck)
}
}
+ private static void longCheck(Runnable assertion, long timeout, TimeUnit timeUnit)
+ {
+ long start = System.currentTimeMillis();
+ for (;;)
+ {
+ try
+ {
+ assertion.run();
+ return;
+ }
+ catch (AssertionError e)
+ {
+ long elapsedMs = System.currentTimeMillis() - start;
+ if (elapsedMs > timeUnit.toMillis(timeout))
+ throw e;
+ else
+ Uninterruptibles.sleepUninterruptibly(5, TimeUnit.MILLISECONDS);
Review comment:
may want to take a look at the `org.apache.cassandra.utils.Retry` class. I created it recently since I didn't find common utils for retries.
```
long maxDurationNanos = timeUnit.toMillis(timeout);
long startNanos = System.nanoTime();
Retry.retryWithBackoffBlocking(Integer.MAX_VALUE, () -> assertion.run(), (cause) -> {
if (!(cause instanceOf AssertionError)
return false;
return (System.nanoTime() - startNanos) <= maxDurationNanos;
});
```
the main differences are:
1) retry doesn't block thread, only result (if you ask for blocking)
2) default to exponential backoff
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393947441
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -240,6 +247,27 @@ private void doCheck(FailCheck testAndFailCheck)
}
}
+ private static void longCheck(Runnable assertion, long timeout, TimeUnit timeUnit)
+ {
+ long start = System.currentTimeMillis();
+ for (;;)
+ {
+ try
+ {
+ assertion.run();
+ return;
+ }
+ catch (AssertionError e)
+ {
+ long elapsedMs = System.currentTimeMillis() - start;
+ if (elapsedMs > timeUnit.toMillis(timeout))
+ throw e;
+ else
+ Uninterruptibles.sleepUninterruptibly(5, TimeUnit.MILLISECONDS);
Review comment:
Thanks for the suggestion. I took a close look at the `Retry` utility.
I think 1) scheduling the check in the other thread does not benefit in this case. Because it needs to block for the check result anyway.
2) the check operation is not resources intensive (read multiple values and compare). Exponential backoff does not help. In fact, the test can check frequently in order to exit quick.
There is no api in `Retry` allows adding a different backup off time yet.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393948198
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -96,6 +93,11 @@ public OutboundCountChecker error(long count, long bytes)
return this;
}
+ public void longCheck(long timeout, TimeUnit timeUnit)
Review comment:
Picking the name mainly for clarity that the check is a long operation that can run for the duration specified.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r398683852
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -197,45 +197,42 @@ public InboundCountChecker error(long count, long bytes)
public void check()
{
- doCheck(Assert::assertEquals);
+ doCheck((message, expected, actual) -> spinAssertEquals(message, expected, actual, 5, TimeUnit.SECONDS));
}
public void check(FailCheck failCheck)
{
- doCheck((message, expect, actual) -> { if (expect != actual) failCheck.accept(message, expect, actual); });
+ doCheck((message, expect, actual) -> { if (!Objects.equals(expect, actual.get())) failCheck.accept(message, expect, actual); });
}
private void doCheck(FailCheck testAndFailCheck)
{
if (checkReceived)
{
- testAndFailCheck.accept("received count values don't match", received, connection.receivedCount());
- testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection.receivedBytes());
+ testAndFailCheck.accept("received count values don't match", received, connection::receivedCount);
+ testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection::receivedBytes);
}
if (checkProcessed)
{
- testAndFailCheck.accept("processed count values don't match", processed, connection.processedCount());
- testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection.processedBytes());
+ testAndFailCheck.accept("processed count values don't match", processed, connection::processedCount);
+ testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection::processedBytes);
}
if (checkExpired)
{
- testAndFailCheck.accept("expired count values don't match", expired, connection.expiredCount());
- testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection.expiredBytes());
+ testAndFailCheck.accept("expired count values don't match", expired, connection::expiredCount);
+ testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection::expiredBytes);
}
if (checkError)
{
- testAndFailCheck.accept("error count values don't match", error, connection.errorCount());
- testAndFailCheck.accept("error bytes values don't match", errorBytes, connection.errorBytes());
+ testAndFailCheck.accept("error count values don't match", error, connection::errorCount);
+ testAndFailCheck.accept("error bytes values don't match", errorBytes, connection::errorBytes);
}
if (checkScheduled)
{
// scheduled cannot relied upon to not race with completion of the task,
- // so if it is currently above the value we expect, sleep for a bit
- if (scheduled < connection.scheduledCount())
- for (int i = 0; i < 10 && scheduled < connection.scheduledCount() ; ++i)
- Uninterruptibles.sleepUninterruptibly(1L, TimeUnit.MILLISECONDS);
- testAndFailCheck.accept("scheduled count values don't match", scheduled, connection.scheduledCount());
- testAndFailCheck.accept("scheduled bytes values don't match", scheduledBytes, connection.scheduledBytes());
+ // so if it is currently above the value we expect, use the spinAssert explicitly.
+ spinAssertEquals("scheduled count values don't match", scheduled, connection::scheduledCount, 5, TimeUnit.SECONDS);
+ spinAssertEquals("scheduled bytes values don't match", scheduledBytes, connection::scheduledBytes, 5, TimeUnit.SECONDS);
Review comment:
If all those failing tests have proven us somthing is that the spin would be needed everywhere. The `doCheck` method with the FailCheck parameter is used in the burn tests to allow the implementor to plug his own notification mechanism for failure. That would not work with the current implementation that use the spin assert.
My approach would be to let the user of that method handle the spinning if needed but I agree it is not a perfect solution.
Otherwise we need to implements a spining that will work for `doCheck()`and `doCheck(FailCheck)`.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r398494290
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -197,45 +197,42 @@ public InboundCountChecker error(long count, long bytes)
public void check()
{
- doCheck(Assert::assertEquals);
+ doCheck((message, expected, actual) -> spinAssertEquals(message, expected, actual, 5, TimeUnit.SECONDS));
}
public void check(FailCheck failCheck)
{
- doCheck((message, expect, actual) -> { if (expect != actual) failCheck.accept(message, expect, actual); });
+ doCheck((message, expect, actual) -> { if (!Objects.equals(expect, actual.get())) failCheck.accept(message, expect, actual); });
}
private void doCheck(FailCheck testAndFailCheck)
{
if (checkReceived)
{
- testAndFailCheck.accept("received count values don't match", received, connection.receivedCount());
- testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection.receivedBytes());
+ testAndFailCheck.accept("received count values don't match", received, connection::receivedCount);
+ testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection::receivedBytes);
}
if (checkProcessed)
{
- testAndFailCheck.accept("processed count values don't match", processed, connection.processedCount());
- testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection.processedBytes());
+ testAndFailCheck.accept("processed count values don't match", processed, connection::processedCount);
+ testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection::processedBytes);
}
if (checkExpired)
{
- testAndFailCheck.accept("expired count values don't match", expired, connection.expiredCount());
- testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection.expiredBytes());
+ testAndFailCheck.accept("expired count values don't match", expired, connection::expiredCount);
+ testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection::expiredBytes);
}
if (checkError)
{
- testAndFailCheck.accept("error count values don't match", error, connection.errorCount());
- testAndFailCheck.accept("error bytes values don't match", errorBytes, connection.errorBytes());
+ testAndFailCheck.accept("error count values don't match", error, connection::errorCount);
+ testAndFailCheck.accept("error bytes values don't match", errorBytes, connection::errorBytes);
}
if (checkScheduled)
{
// scheduled cannot relied upon to not race with completion of the task,
- // so if it is currently above the value we expect, sleep for a bit
- if (scheduled < connection.scheduledCount())
- for (int i = 0; i < 10 && scheduled < connection.scheduledCount() ; ++i)
- Uninterruptibles.sleepUninterruptibly(1L, TimeUnit.MILLISECONDS);
- testAndFailCheck.accept("scheduled count values don't match", scheduled, connection.scheduledCount());
- testAndFailCheck.accept("scheduled bytes values don't match", scheduledBytes, connection.scheduledBytes());
+ // so if it is currently above the value we expect, use the spinAssert explicitly.
+ spinAssertEquals("scheduled count values don't match", scheduled, connection::scheduledCount, 5, TimeUnit.SECONDS);
+ spinAssertEquals("scheduled bytes values don't match", scheduledBytes, connection::scheduledBytes, 5, TimeUnit.SECONDS);
Review comment:
NIT: I would have used `testAndFailCheck` for ensuring that the behavior is the one expected by the `FailCheck` argument.
In practice, unless I missed something, this code is only called from `ConnectionTest` so there will be no change of behavior.
I can change that on commit if you want to.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393926560
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -96,6 +93,11 @@ public OutboundCountChecker error(long count, long bytes)
return this;
}
+ public void longCheck(long timeout, TimeUnit timeUnit)
Review comment:
why not just `check`? you don't have a signature conflict so could have the same API but with the duration.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393923257
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -240,6 +247,27 @@ private void doCheck(FailCheck testAndFailCheck)
}
}
+ private static void longCheck(Runnable assertion, long timeout, TimeUnit timeUnit)
+ {
+ long start = System.currentTimeMillis();
Review comment:
`System.currentTimeMillis` is free to go backwards (its not monotonic, this is system time) so its normally best to use `.nanoTime()` (CPU time).
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r398683852
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -197,45 +197,42 @@ public InboundCountChecker error(long count, long bytes)
public void check()
{
- doCheck(Assert::assertEquals);
+ doCheck((message, expected, actual) -> spinAssertEquals(message, expected, actual, 5, TimeUnit.SECONDS));
}
public void check(FailCheck failCheck)
{
- doCheck((message, expect, actual) -> { if (expect != actual) failCheck.accept(message, expect, actual); });
+ doCheck((message, expect, actual) -> { if (!Objects.equals(expect, actual.get())) failCheck.accept(message, expect, actual); });
}
private void doCheck(FailCheck testAndFailCheck)
{
if (checkReceived)
{
- testAndFailCheck.accept("received count values don't match", received, connection.receivedCount());
- testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection.receivedBytes());
+ testAndFailCheck.accept("received count values don't match", received, connection::receivedCount);
+ testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection::receivedBytes);
}
if (checkProcessed)
{
- testAndFailCheck.accept("processed count values don't match", processed, connection.processedCount());
- testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection.processedBytes());
+ testAndFailCheck.accept("processed count values don't match", processed, connection::processedCount);
+ testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection::processedBytes);
}
if (checkExpired)
{
- testAndFailCheck.accept("expired count values don't match", expired, connection.expiredCount());
- testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection.expiredBytes());
+ testAndFailCheck.accept("expired count values don't match", expired, connection::expiredCount);
+ testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection::expiredBytes);
}
if (checkError)
{
- testAndFailCheck.accept("error count values don't match", error, connection.errorCount());
- testAndFailCheck.accept("error bytes values don't match", errorBytes, connection.errorBytes());
+ testAndFailCheck.accept("error count values don't match", error, connection::errorCount);
+ testAndFailCheck.accept("error bytes values don't match", errorBytes, connection::errorBytes);
}
if (checkScheduled)
{
// scheduled cannot relied upon to not race with completion of the task,
- // so if it is currently above the value we expect, sleep for a bit
- if (scheduled < connection.scheduledCount())
- for (int i = 0; i < 10 && scheduled < connection.scheduledCount() ; ++i)
- Uninterruptibles.sleepUninterruptibly(1L, TimeUnit.MILLISECONDS);
- testAndFailCheck.accept("scheduled count values don't match", scheduled, connection.scheduledCount());
- testAndFailCheck.accept("scheduled bytes values don't match", scheduledBytes, connection.scheduledBytes());
+ // so if it is currently above the value we expect, use the spinAssert explicitly.
+ spinAssertEquals("scheduled count values don't match", scheduled, connection::scheduledCount, 5, TimeUnit.SECONDS);
+ spinAssertEquals("scheduled bytes values don't match", scheduledBytes, connection::scheduledBytes, 5, TimeUnit.SECONDS);
Review comment:
If all those failing tests have proven us somthing is that the spin would be needed everywhere. The `doCheck` method with the FailCheck parameter is used in the burn tests to allow the implementor to plug his own notification mechanism for failure. That would not work with the current implementation that use the spin assert.
My approach would be to let the user of that method handle the spinning if needed but I agree it is not a perfect solution.
Otherwise we need to implements a spining that will work for `doCheck()`and `doCheck(FailCheck)`.
What do you think?
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r398665708
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -197,45 +197,42 @@ public InboundCountChecker error(long count, long bytes)
public void check()
{
- doCheck(Assert::assertEquals);
+ doCheck((message, expected, actual) -> spinAssertEquals(message, expected, actual, 5, TimeUnit.SECONDS));
}
public void check(FailCheck failCheck)
{
- doCheck((message, expect, actual) -> { if (expect != actual) failCheck.accept(message, expect, actual); });
+ doCheck((message, expect, actual) -> { if (!Objects.equals(expect, actual.get())) failCheck.accept(message, expect, actual); });
}
private void doCheck(FailCheck testAndFailCheck)
{
if (checkReceived)
{
- testAndFailCheck.accept("received count values don't match", received, connection.receivedCount());
- testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection.receivedBytes());
+ testAndFailCheck.accept("received count values don't match", received, connection::receivedCount);
+ testAndFailCheck.accept("received bytes values don't match", receivedBytes, connection::receivedBytes);
}
if (checkProcessed)
{
- testAndFailCheck.accept("processed count values don't match", processed, connection.processedCount());
- testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection.processedBytes());
+ testAndFailCheck.accept("processed count values don't match", processed, connection::processedCount);
+ testAndFailCheck.accept("processed bytes values don't match", processedBytes, connection::processedBytes);
}
if (checkExpired)
{
- testAndFailCheck.accept("expired count values don't match", expired, connection.expiredCount());
- testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection.expiredBytes());
+ testAndFailCheck.accept("expired count values don't match", expired, connection::expiredCount);
+ testAndFailCheck.accept("expired bytes values don't match", expiredBytes, connection::expiredBytes);
}
if (checkError)
{
- testAndFailCheck.accept("error count values don't match", error, connection.errorCount());
- testAndFailCheck.accept("error bytes values don't match", errorBytes, connection.errorBytes());
+ testAndFailCheck.accept("error count values don't match", error, connection::errorCount);
+ testAndFailCheck.accept("error bytes values don't match", errorBytes, connection::errorBytes);
}
if (checkScheduled)
{
// scheduled cannot relied upon to not race with completion of the task,
- // so if it is currently above the value we expect, sleep for a bit
- if (scheduled < connection.scheduledCount())
- for (int i = 0; i < 10 && scheduled < connection.scheduledCount() ; ++i)
- Uninterruptibles.sleepUninterruptibly(1L, TimeUnit.MILLISECONDS);
- testAndFailCheck.accept("scheduled count values don't match", scheduled, connection.scheduledCount());
- testAndFailCheck.accept("scheduled bytes values don't match", scheduledBytes, connection.scheduledBytes());
+ // so if it is currently above the value we expect, use the spinAssert explicitly.
+ spinAssertEquals("scheduled count values don't match", scheduled, connection::scheduledCount, 5, TimeUnit.SECONDS);
+ spinAssertEquals("scheduled bytes values don't match", scheduledBytes, connection::scheduledBytes, 5, TimeUnit.SECONDS);
Review comment:
It is probably better to use `spinAssertEquals` explicitly. If you strongly prefer to use `testAndFailCheck`, I am cool with it.
The spin equality assertion was used because
- the original code that Benedict wrote did it explicitly using a for loop. The assertion is likely to require multiple retries.
- the parameter type `FailCheck` for `doCheck()` suggests it can accept any `FailCheck` impl. We cannot assume the input check is always spin assertion based (, although it is only used in `ConnectionTest` as of now). If a custom non-spin assertion is passed, the check can fail.
- the error message, i.e. "scheduled count values don't match", suggests the purpose is to assert values are equal. So `spinAssertEquals` does the job.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393925866
##########
File path: test/unit/org/apache/cassandra/net/ConnectionTest.java
##########
@@ -487,13 +487,13 @@ public long serializedSize(Object o, int version)
.overload ( 0, 0)
.expired ( 0, 0)
.error ( 10, 10 * message.serializedSize(version))
- .check();
+ .longCheck( 5, SECONDS);
Review comment:
would also be good to document why not check; you say it in the JIRA but a code comment is easier to future readers.
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #470:
CASSANDRA-15630 fix testSerializeError
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #470: CASSANDRA-15630 fix testSerializeError
URL: https://github.com/apache/cassandra/pull/470#discussion_r393968395
##########
File path: test/unit/org/apache/cassandra/net/ConnectionUtils.java
##########
@@ -240,6 +247,31 @@ private void doCheck(FailCheck testAndFailCheck)
}
}
+ /**
+ * Perform the {@param assertion} within the specified duration repeatly until a success.
+ * Otherwise, it times out and re-throws the {@link AssertionError} from the {@param assertion}.
+ */
+ private static void longCheck(Runnable assertion, long timeout, TimeUnit timeUnit)
+ {
+ long startNano = System.nanoTime();
+ for (;;)
+ {
+ try
+ {
+ assertion.run();
+ return;
+ }
+ catch (AssertionError e)
+ {
+ long elapsedNano = System.nanoTime() - startNano;
+ if (elapsedNano > timeUnit.toNanos(timeout))
Review comment:
sorry I didn't comment sooner, I left it in my example; it would be best to compute `timeUnit.toNanos(timeout)` once outside the loop
----------------------------------------------------------------
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: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org