dcapwell commented on code in PR #57:
URL: https://github.com/apache/cassandra-accord/pull/57#discussion_r1302227437
##########
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:
> Your examples are comparing this patch in its current form with the patch
in its form when this disagreement arose,
No, I am not. I am replying to your comments and arguing they are not
really valid; I give such examples as `return () -> time.next(rs);` vs `return
time.asSupplier(rs);` which actively compares pre-feedback and post-feedback.
I also reply to your comment about `Cluster` and `NodeSink` where the change
doesn't have anything to do with `Gen` vs `Supplier`, the change was mostly for
debug logging reasons.
> I am suggesting we move forwards.
What are the open issues before a +1 is given? It is not clear what
feedback is required vs optional as the only active threads I see is
https://github.com/apache/cassandra-accord/pull/57#discussion_r1301845319
(small change to a single method) and this thread, so I want to make sure I
understand the scope of work remaining.
> This is mixing styles in my book.
This is not at all true though. `trunk` branch is currently
8c7a3c9ef4209d635b186189e17a2d9e728e9871, which has the following call paths
that this PR touches that are under conflict
* `BurnTest` `nowSupplier`:
https://github.com/apache/cassandra-accord/blob/8c7a3c9ef4209d635b186189e17a2d9e728e9871/accord-core/src/test/java/accord/burn/BurnTest.java#L202-L206.
In this example we use a `Supplier<LongSupplier>` as we create one per node,
and `Node` uses `LongSupplier` for time. Within this supplier we do `return ()
-> Math.max(0, delayQueue.nowInMillis() + (jitter.nextInt(10) - 5));`. Before
applying the feedback you asked for, we still returned a
`Supplier<LongSupplier>` and replaced the body to be `return () -> Math.max(0,
queue.nowInMillis() + jitter.nextLong(forked));`. So, the only change before was
```
// before
jitter.nextInt(10) - 5
// after
jitter.nextLong(forked)
```
Before the logic was stateless, but we moved to a stateful model to express
"runs" of long jitter, so moved from calling `RandomSource` to
`FrequentLargeRange`. I don't see how this logic was not within the style
before and how it is now, or how future changes are any different.
* `RandomDelayQueue` `add`:
https://github.com/apache/cassandra-accord/blob/8c7a3c9ef4209d635b186189e17a2d9e728e9871/accord-core/src/test/java/accord/impl/basic/RandomDelayQueue.java#L102
```
// before
random.nextInt(500)
// after
TimeUnit.NANOSECONDS.toMillis(jitterInNano.nextLong(random))
// after cleanup
jitterInMillis.nextLong(random)
```
Again, this moved from a stateless source of random to a stateful one and
the local style is not really changed.
* `NodeSink` `send`:
https://github.com/apache/cassandra-accord/blob/8c7a3c9ef4209d635b186189e17a2d9e728e9871/accord-core/src/test/java/accord/impl/basic/NodeSink.java#L65-L79.
This class would delegate delivery and timeouts to the `PendingQueue` and only
used random to change the delay; which is mostly unchanged in my patch
(before/after). The core change to this class is making it so we don't
`DELIVER` every time and conditionally deliver; which is the feature request
that this JIRA is scoped to do. So
```
// before
parent.add(self, to, messageId, send); // deliver
// after
Action action = partitioned(to) ? Action.DROP_PARTITIONED
// call actions() per node so each
one has different "runs" state
: nodeActions.computeIfAbsent(to,
ignore -> actions()).next(rs);
// can be a DELIVER, DELIVER_WITH_FAILURE, FAILURE, DROP, or DROP_PARTITIONED
```
The other change was that we don't use "cpu scheduling delay" (as we talked
about) and to use a "network latency delay", so again
```
// before
parent.add(self, to, messageId, send); // deliver
// after
parent.add(packet, networkJitterNanos(to), TimeUnit.NANOSECONDS);
```
I struggle to see how I change the "style" (outside of `NodeSink` which went
from nothing to something...). If this is the argument imposed in this PR I
have no clue how to deal with
https://github.com/apache/cassandra-accord/pull/58 which actually changes the
style (moving away from `RandomSource` in favor of `Supplier` and interfaces
that extend it. This moves away from the top level classes *owning* the random
and how to do things in favor of a new abstraction)!
> this is very different to my style of authorship
I have to push back on this... Apache Accord style guide is the Apache
Cassandra style guide, which does not document what you are imposing; that is
your personal style. Our current rules state that you have to have `clearly
expressed and reasonable technical grounds` for such a pushback; we are
supposed to work as a team. If we randomly start changing the rules without
voting and documenting, then we are not able to work as a team.
> since that is entirely sufficient here
Your patch exposes a new interface and move away from `RandomSource` within
the context of this argument (since we are talking about the code calling
`.next`)... you use `RandomSource` to create a `Supplier`, which is a new style
you wish to move forward with in
https://github.com/apache/cassandra-accord/pull/58. In this patch I use
`FrequentLargeRange`, so is your argument here that `FrequentLargeRange` should
be merged into a function call within `RandomSource`? If so this is the first
time I have seen such an argument imposed here, the closest was the comment
about `Gens.bools().biasedRepeatingRuns(ratio)` vs
`rs.biasedRepeatingRuns(ratio)`; to me this feels like a very different
conversation than `Supplier` vs `Gen`.
> This is totally fine when authoring your own classes, but it's less well
received by authors of their own classes that they're still maintaining, in
which consistency of style will be harmed.
The code is owned by Apache and we all are expected to work together on it;
neither of us *own* any file.
I still hold by my pushback above, I don't see how I change the style of the
code on trunk. We can not use `RandomSource` directly at each callback as part
of the requirement for this JIRA is to add some set of state to improve the
randomness, that state is currently stored in a `Gen` and in your other PR you
want to introduce `Supplier`. I would argue that my style is closer to the
current style than ours as the *owner* of the `RandomSource` is the top level
class.
> I don't think this is the first time we've discussed our conflicting
approaches to test authorship, so this shouldn't be a huge surprise?
You have told me before you don't like `Gen` but have not given actual
reasons. The signature is also present (just different named) in Simulator, so
it's hard to see what your argument actually is.
We talked about this in eviction, where I did not force you to match my
style, I pointed out that you were duplicating code and had bugs, so you could
fix your code (and I showed you how) or reuse. You had expressed there again
that you don't like `Gen` and would prefer to hand roll your own logic, which I
was fine with.
> If your style and its components begin to weave their way amongst my style
of authorship, then suddenly I have to have opinions about how your API is
structured, it's inescapable.
And the same seems to be true for me it seems. I had expressed my feedback
in your patch and am +0 for you to introduce `Supplier`; I don't want to block
you because of my preferences, but you actively choose to block me due to
yours. I don't agree with your choice, but I will not block you on it as it is
still "correct". I will ask you to fix or remove code that isn't (which I
pointed out in your PR), but since `Supplier` works, I won't block it.
So yes, we are different here. I do not try to force you into my style, but
you wish to force me, then argue that we should live in peace doing our own
things...
--
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]
