dcapwell commented on code in PR #57:
URL: https://github.com/apache/cassandra-accord/pull/57#discussion_r1302130156
##########
accord-core/src/test/java/accord/burn/random/FrequentLargeRange.java:
##########
@@ -18,59 +18,121 @@
package accord.burn.random;
-import accord.utils.Invariants;
+import accord.utils.Gen;
+import accord.utils.Gen.LongGen;
+import accord.utils.Gens;
import accord.utils.RandomSource;
-public class FrequentLargeRange implements RandomLong
+import java.time.Duration;
+import java.util.concurrent.TimeUnit;
+
+public class FrequentLargeRange implements LongGen
{
- private final RandomLong small, large;
- private final double ratio;
- private final int steps;
- private final double lower, upper;
- private int run = -1;
- private long smallCount = 0, largeCount = 0;
-
- public FrequentLargeRange(RandomLong small, RandomLong large, double ratio)
+ private final LongGen small, large;
+ private final Gen<Boolean> runs;
+
+ public FrequentLargeRange(LongGen small, LongGen large, double ratio)
{
- Invariants.checkArgument(ratio > 0 && ratio <= 1);
this.small = small;
this.large = large;
- this.ratio = ratio;
- this.steps = (int) (1 / ratio);
- this.lower = ratio * .8;
- this.upper = ratio * 1.2;
+ this.runs = Gens.bools().biasedRepeatingRuns(ratio);
Review Comment:
> Cluster has been refactored to NodeSink,
do you mean the moving the `partition` logic to `NodeSink`? That doesn't
have anything to do with `Gen` vs `Supplier` argument and was done to keep the
logic clear and easier for debug logging; I added logging that shows if we
chose to drop the msg vs we dropped due to it being partitioned.
> which alongside BurnTest and RandomDelayQueue were modified in this way
prior to your reverting them, which happened after this debate was initiated.
* `BurnTest` always returned a Supplier, it was just a lambda now its a
function call
```
return () -> time.next(rs);
// vs now
return time.asSupplier(rs);
```
The change in this patch was to reusing existing logic to implement a
behavior we wanted, to add jitter for time...
* `RandomDelayQueue` never exposed `Gen`, it just used an existing type
`accord.burn.random.FrequentLargeRange` and called it using `.next`. A better
argument in this case is that the code was cleaner to extract the mapping logic
to the constructor and use a `LongSupplier` in this case, so in this case I
would agree the `LongSupplier` is cleaner code than the previous, hence why I
didn't bother to push back here.
```
// before
TimeUnit.NANOSECONDS.toMillis(jitterNanos.next(rs))
// after
jitterMillis.getAsLong()
```
> Though I fear we’re going to continue to revisit this.
I agree I feel this is going to keep coming up. The examples you gave never
exposed `Gen`, they just reusing existing code... so the pushback to me was
that when using existing code that happens to have a `Gen` interface, and
intersects classes that you feel must be a given way, then must use a style you
want, that is not documented, fleshed out, or voted on... so as a contributor
to this project I have no idea when the next argument will pop up.
What's also unexpected to me about this is that Simulator has a similar
style to `Gen` (it has both styles); you supply the `RandomSource`[1]. We even
spoke months back about one day unifying these so that Simulator extends the
logic in Accord. So from my view this feedback came out of no where and became
a blocking comment... so when will the next one come up?
[1]
```
// see
org.apache.cassandra.simulator.ClusterSimulation.Builder#schedulerFactory
static SchedulerFactory schedulerFactory(RunnableActionScheduler.Kind...
kinds)
{
return (random) -> {
switch (Choices.random(random, kinds).choose(random))
{
default: throw new AssertionError();
case SEQUENTIAL: return new
RunnableActionScheduler.Sequential();
case UNIFORM: return new
RunnableActionScheduler.RandomUniform(random);
case RANDOM_WALK: return new
RunnableActionScheduler.RandomWalk(random);
}
};
}
```
##########
accord-core/src/test/java/accord/burn/random/FrequentLargeRange.java:
##########
@@ -18,59 +18,121 @@
package accord.burn.random;
-import accord.utils.Invariants;
+import accord.utils.Gen;
+import accord.utils.Gen.LongGen;
+import accord.utils.Gens;
import accord.utils.RandomSource;
-public class FrequentLargeRange implements RandomLong
+import java.time.Duration;
+import java.util.concurrent.TimeUnit;
+
+public class FrequentLargeRange implements LongGen
{
- private final RandomLong small, large;
- private final double ratio;
- private final int steps;
- private final double lower, upper;
- private int run = -1;
- private long smallCount = 0, largeCount = 0;
-
- public FrequentLargeRange(RandomLong small, RandomLong large, double ratio)
+ private final LongGen small, large;
+ private final Gen<Boolean> runs;
+
+ public FrequentLargeRange(LongGen small, LongGen large, double ratio)
{
- Invariants.checkArgument(ratio > 0 && ratio <= 1);
this.small = small;
this.large = large;
- this.ratio = ratio;
- this.steps = (int) (1 / ratio);
- this.lower = ratio * .8;
- this.upper = ratio * 1.2;
+ this.runs = Gens.bools().biasedRepeatingRuns(ratio);
Review Comment:
> Cluster has been refactored to NodeSink,
do you mean the moving the `partition` logic to `NodeSink`? That doesn't
have anything to do with `Gen` vs `Supplier` argument and was done to keep the
logic clear and easier for debug logging; I added logging that shows if we
chose to drop the msg vs we dropped due to it being partitioned.
> which alongside BurnTest and RandomDelayQueue were modified in this way
prior to your reverting them, which happened after this debate was initiated.
* `BurnTest` always returned a Supplier, it was just a lambda now its a
function call
```
return () -> time.next(rs);
// vs now
return time.asSupplier(rs);
```
The change in this patch was to reusing existing logic to implement a
behavior we wanted, to add jitter for time...
* `RandomDelayQueue` never exposed `Gen`, it just used an existing type
`accord.burn.random.FrequentLargeRange` and called it using `.next`. A better
argument in this case is that the code was cleaner to extract the mapping logic
to the constructor and use a `LongSupplier` in this case, so in this case I
would agree the `LongSupplier` is cleaner code than the previous, hence why I
didn't bother to push back here.
```
// before
TimeUnit.NANOSECONDS.toMillis(jitterNanos.next(rs))
// after
jitterMillis.getAsLong()
```
> Though I fear we’re going to continue to revisit this.
I agree I feel this is going to keep coming up. The examples you gave never
exposed `Gen`, they just reusing existing code... so the pushback to me was
that when using existing code that happens to have a `Gen` interface, and
intersects classes that you feel must be a given way, then must use a style you
want, that is not documented, fleshed out, or voted on... so as a contributor
to this project I have no idea when the next argument will pop up.
What's also unexpected to me about this is that Simulator has a similar
style to `Gen` (it has both styles); you supply the `RandomSource`[1]. We even
spoke months back about one day unifying these so that Simulator extends the
logic in Accord. So from my view this feedback came out of no where and became
a blocking comment... so when will the next one come up?
[1]
```
// see
org.apache.cassandra.simulator.ClusterSimulation.Builder#schedulerFactory
static SchedulerFactory schedulerFactory(RunnableActionScheduler.Kind...
kinds)
{
return (random) -> {
switch (Choices.random(random, kinds).choose(random))
{
default: throw new AssertionError();
case SEQUENTIAL: return new
RunnableActionScheduler.Sequential();
case UNIFORM: return new
RunnableActionScheduler.RandomUniform(random);
case RANDOM_WALK: return new
RunnableActionScheduler.RandomWalk(random);
}
};
}
```
--
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]
