dcapwell commented on code in PR #28:
URL: https://github.com/apache/cassandra-accord/pull/28#discussion_r1100814281
##########
accord-core/src/test/java/accord/impl/basic/DelayedExecutorService.java:
##########
@@ -0,0 +1,107 @@
+package accord.impl.basic;
+
+import accord.burn.random.Decision;
+import accord.burn.random.IntRange;
+import accord.burn.random.RandomInt;
+import accord.burn.random.SegmentedIntRange;
+import org.apache.cassandra.concurrent.FutureTask;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.AbstractExecutorService;
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+public class DelayedExecutorService extends AbstractExecutorService
+{
+ private final PendingQueue pending;
+ private final Random random;
+ private final RandomInt jitterInNano;
+
+ public DelayedExecutorService(PendingQueue pending, Random random)
+ {
+ this.pending = pending;
+ this.random = random;
+ this.jitterInNano = new SegmentedIntRange(
+ new IntRange(microToNanos(0), microToNanos(50)),
+ new IntRange(microToNanos(50), msToNanos(5)),
+ // this is different from Apache Cassandra Simulator as this
is computed differently for each executor
Review Comment:
I have been working on unit tests and finding this class is more complex
than expected... I had to add a concept of "resample" so we keep adding more
attempts to the loop when the target ratio isn't met... I start with 100k
samples and in cases like the following, I need `21473837` resamples (`21473837
* 100 = 2,147,383,700` attempts needed).
```
int maxSmall = 78782771;
int maxLarge = 79722613;
int smallRatio = 14;
SegmentedRandomWalkPeriod period = new SegmentedRandomWalkPeriod(new
IntRange(0, maxSmall), new IntRange(0, maxLarge), smallRatio, rs);
```
It seems that when it gets biased towards large it takes too many samples
before getting back inline... It also requires that `large` overlaps with
`small` (though I think this is fixable), which makes this class less desirable
I think; I personally feel that the existing `SegmentedIntRange` is better as
it stays within the expected range.
If you want to take a look at the tests, they are in
`accord.burn.random.SegmentedRandomRangeTest`. `disjoint` tests
`SegmentedIntRange` and `overlap` tests `Decision.SegmentedRandomWalkPeriod`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
