osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [All][RNG] Fix minor design issue (Was: commons-rng git commit: [...] deprecations. [...])


On Thu, Aug 9, 2018 at 8:47 AM Gary Gregory <garydgregory@xxxxxxxxx> wrote:

>
>
> On Thu, Aug 9, 2018 at 7:25 AM Rob Tompkins <chtompki@xxxxxxxxx> wrote:
>
>>
>>
>> > On Aug 9, 2018, at 9:17 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>> wrote:
>> >
>> > On Thu, 9 Aug 2018 08:43:34 -0400, Rob Tompkins wrote:
>> >>> On Aug 9, 2018, at 8:38 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>> wrote:
>> >>>
>> >>> On Thu, 9 Aug 2018 08:22:32 -0400, Rob Tompkins wrote:
>> >>>> Are we instead trying to remove the “extends” from all of those
>> classes?
>> >>>
>> >>> I'm not sure what you mean, sorry.
>> >>>
>> >>> "SamplerBase" was meant has a utility/shortcut/boiler-plate in order
>> >>> to copy the "rng" argument in one place, instead of having a field
>> >>> in each sampler class. It is purely internal to this library (in C++,
>> >>> inheritance would have been "private”).
>> >>
>> >> Are we trying to remove SamplerBase all together?
>> >
>> > That would break BC the same way.
>> > I wouldn't mind since the shortcut is not really significant.
>> >
>> > But my reasoning would be the same: correct usage does not refer
>> > to these methods so the fix could be done now, with minimal risk
>> > (due to low usage of a new component).
>> >
>> >>>> My thought was to override the method and javadoc locally (in the
>> >>>> actual subclass) so that we can give reasonable deprecation messages,
>> >>>> but that’s only a thought. I’m ok with whatever.
>> >
>> > It's overkill in this context.
>> > We shouldn't be rigid about the "no BC in minor release" rule;
>>
>> I’m a tad confused here. I thought that the whole point of everything
>> that we do in terms of the way we version and name packages
>> was specifically driven at the fact that we’re completely rigid about “no
>> BC breakage in minor releases.”
>>
>> I’ve not been around the project long enough to know this with certainty,
>> but that definitely is the vibe that I get. I would think that such RCs
>> would generally get -1’s from people.
>>
>> Generally I’m open to whatever. I’m just operating on what I perceive to
>> be precedent.
>>
>> @Gary - what’s the call here on the flexibility of BC incompatible
>> changes in a minor release?
>>
>
> BC is binary and we should not break stacks. You really don't want to
> create jar hell for our users. Think about deep transitive dependencies. An
> end use or developer is not going to care 2 cents about "correct vs
> incorrect API usage" when a component depends on this depends on that and
> so on, ends up breaking his or her whole build because a low level
> component has decided that breaking BC was OK.
>

I'll also note that I am not close enough to this component to opine on the
quality of the API or the workarounds discussed. My point is only "Don't
create jar hell; don't break BC."

Gary



>
> Gary
>
>
>> > enforcing it is worse for everybody:
>> > * bona fide users will be forced to modify their source and
>> >   recompile in order to benefit from the performance boost
>> >   introduced in version 1.1, and
>> > * bad usage (if it exists at all) could continue unsuspected.
>> >
>> >>> The message would aim at the wrong target: for developers of this
>> >>> library, there is no deprecation (the boiler-plate code is there
>> >>> to be used); for application developers, the class should not be
>> >>> there to be used (hence: package private).
>> >>
>> >> Then, I guess, we should simply state that we want to eventually
>> >> delete these methods and they should not be consumed. Yeah?
>> >
>> > No (cf. above); I propose to delete them *now*.
>> >
>> >> Like I
>> >> said…I’m up for whatever. I’m just trying to balance yours and Gary’s
>> >> points and find a good middle ground that everyone can live with.
>> >
>> > Which are the two sides for which you try to find a middle ground?
>> > At any rate it would not be "good" if it makes developers and users
>> > unhappy for a case that probably doesn't exist out there.
>> > Tools such as Clirr are a great help, but they shouldn't induce us
>> > to take the wrong course only to have the pleasure to contemplate a
>> > clean report. ;-)
>> > Thanks to Clirr (and Gary), we've become aware of a design issue.
>> > We can decide to resolve it as it was done in the past, with IMHO
>> > zero inconvenience (except for the Clirr "unclean" report).
>> >
>> > Regards,
>> > Gilles
>> >
>> >> -Rob
>> >>
>> >>>
>> >>> Gilles
>> >>>
>> >>>> -Rob
>> >>>>
>> >>>>> On Aug 9, 2018, at 7:02 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>> wrote:
>> >>>>>
>> >>>>> Hi all.
>> >>>>>
>> >>>>> On Wed, 08 Aug 2018 19:59:24 +0200, Gilles wrote:
>> >>>>>> Hello Rob.
>> >>>>>>
>> >>>>>> On Wed, 8 Aug 2018 12:44:16 -0400, Rob Tompkins wrote:
>> >>>>>>> @Gilles - thoughts here?? Just kinda what I was thinking, but I’m
>> >>>>>>> only a +0 on this change. So, if you want to revert it before
>> going up
>> >>>>>>> with 1.1, that’s fine.
>> >>>>>>
>> >>>>>> I don't understand this change after what I answered to Gary's
>> strange
>> >>>>>> proposal.
>> >>>>>> The comment is *wrong*.  As I said previously, the base class
>> provides
>> >>>>>> boiler-plate code; that is *not* deprecated.  The issue is that
>> those
>> >>>>>> methods were not meant to be used further down the hierarchy (in
>> user's
>> >>>>>> subclasses). [And now, furthermore, they are not used anymore in
>> class
>> >>>>>> "PoissonSampler", following the change in RNG-50 (delegation to
>> other
>> >>>>>> classes).]
>> >>>>>> The sampler classes should have been made "final"; but now, this
>> change
>> >>>>>> would also be "breaking" (even though I doubt about legitimate
>> use-cases
>> >>>>>> for inherithing from the sampler implementations).
>> >>>>>
>> >>>>> Assuming it's unlikely that application developers would have
>> >>>>> * called protected methods of "SamplerBase",[1]
>> >>>>> * create subclasses of "SamplerBase",[1]
>> >>>>> * create subclasses of the sampler classes,[1]
>> >>>>> I suggest to
>> >>>>> * make "SamplerBase" package private
>> >>>>> * make the sampler clases "final".
>> >>>>>
>> >>>>> The above are the *less* intrusive fixes, as they would only
>> >>>>> potentially impact application developers by making them aware
>> >>>>> that they have made incorrect usage of an *inadvertently*
>> >>>>> public API.  Correct usage will not be impacted even though
>> >>>>> the change is not BC.[2]
>> >>>>>
>> >>>>> Any objection to that fix?[3]
>> >>>>>
>> >>>>> Regards,
>> >>>>> Gilles
>> >>>>>
>> >>>>> [1] Rationale: (a) There is no reason to do that and
>> >>>>>             (b) "Commons RNG" isn't much used at this point:
>> >>>>>
>> http://mvnrepository.com/artifact/org.apache.commons/commons-rng-sampling/usages
>> >>>>> [2] I recall that a similar situation (BC breaking in a minor
>> >>>>>  release) occurred in CM, at a time where the number of
>> >>>>>  potential users was much larger.
>> >>>>> [3] I'll add a prominent warning in the changelog to the effect
>> >>>>>  that people wanting to continue with incorrect usage of the
>> >>>>>  samplers should not upgrade. ;-)
>> >>>>>
>> >>>>>>
>> >>>>>> Please revert.
>> >>>>>>
>> >>>>>> Thanks,
>> >>>>>> Gilles
>> >>>>>>
>> >>>>>>>
>> >>>>>>> -Rob
>> >>>>>>>
>> >>>>>>>> On Aug 8, 2018, at 12:42 PM, chtompki@xxxxxxxxxx wrote:
>> >>>>>>>>
>> >>>>>>>> Repository: commons-rng
>> >>>>>>>> Updated Branches:
>> >>>>>>>> refs/heads/1.1 50b984b1d -> f8159f28a
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> Adding PoissonSampler deprecations. Use the correct faster
>> public methods
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-rng/repo
>> >>>>>>>> Commit:
>> http://git-wip-us.apache.org/repos/asf/commons-rng/commit/f8159f28
>> >>>>>>>> Tree:
>> http://git-wip-us.apache.org/repos/asf/commons-rng/tree/f8159f28
>> >>>>>>>> Diff:
>> http://git-wip-us.apache.org/repos/asf/commons-rng/diff/f8159f28
>> >>>>>>>>
>> >>>>>>>> Branch: refs/heads/1.1
>> >>>>>>>> Commit: f8159f28a52197d0e7b55e39b115702147cf57a0
>> >>>>>>>> Parents: 50b984b
>> >>>>>>>> Author: Rob Tompkins <chtompki@xxxxxxxxx>
>> >>>>>>>> Authored: Wed Aug 8 12:42:45 2018 -0400
>> >>>>>>>> Committer: Rob Tompkins <chtompki@xxxxxxxxx>
>> >>>>>>>> Committed: Wed Aug 8 12:42:45 2018 -0400
>> >>>>>>>>
>> >>>>>>>>
>> ----------------------------------------------------------------------
>> >>>>>>>> .../sampling/distribution/PoissonSampler.java   | 42
>> ++++++++++++++++++++
>> >>>>>>>> 1 file changed, 42 insertions(+)
>> >>>>>>>>
>> ----------------------------------------------------------------------
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> http://git-wip-us.apache.org/repos/asf/commons-rng/blob/f8159f28/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>> >>>>>>>>
>> ----------------------------------------------------------------------
>> >>>>>>>> diff --git
>> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>> >>>>>>>> index d0733ba..29d0e4e 100644
>> >>>>>>>> ---
>> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>> >>>>>>>> +++
>> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>> >>>>>>>> @@ -67,6 +67,48 @@ public class PoissonSampler
>> >>>>>>>>      return poissonSampler.sample();
>> >>>>>>>>  }
>> >>>>>>>>
>> >>>>>>>> +    /**
>> >>>>>>>> +     * @return a random value from a uniform distribution in the
>> >>>>>>>> +     * interval {@code [0, 1)}.
>> >>>>>>>> +     * @deprecated - one should be using the {@link
>> PoissonSampler#sample()} method,
>> >>>>>>>> +     * as it is considerably faster.
>> >>>>>>>> +     */
>> >>>>>>>> +    @Deprecated
>> >>>>>>>> +    protected double nextDouble() {
>> >>>>>>>> +        return super.nextDouble();
>> >>>>>>>> +    }
>> >>>>>>>> +
>> >>>>>>>> +    /**
>> >>>>>>>> +     * @return a random {@code int} value.
>> >>>>>>>> +     * @deprecated - one should be using the {@link
>> PoissonSampler#sample()} method,
>> >>>>>>>> +     * as it is considerably faster.
>> >>>>>>>> +     */
>> >>>>>>>> +    @Deprecated
>> >>>>>>>> +    protected int nextInt() {
>> >>>>>>>> +        return super.nextInt();
>> >>>>>>>> +    }
>> >>>>>>>> +
>> >>>>>>>> +    /**
>> >>>>>>>> +     * @param max Upper bound (excluded).
>> >>>>>>>> +     * @return a random {@code int} value in the interval
>> {@code [0, max)}.
>> >>>>>>>> +     * @deprecated - one should be using the {@link
>> PoissonSampler#sample()} method,
>> >>>>>>>> +     *      * as it is considerably faster.
>> >>>>>>>> +     */
>> >>>>>>>> +    @Deprecated
>> >>>>>>>> +    protected int nextInt(int max) {
>> >>>>>>>> +        return super.nextInt(max);
>> >>>>>>>> +    }
>> >>>>>>>> +
>> >>>>>>>> +    /**
>> >>>>>>>> +     * @return a random {@code long} value.
>> >>>>>>>> +     * @deprecated - one should be using the {@link
>> PoissonSampler#sample()} method,
>> >>>>>>>> +     *      * as it is considerably faster.
>> >>>>>>>> +     */
>> >>>>>>>> +    @Deprecated
>> >>>>>>>> +    protected long nextLong() {
>> >>>>>>>> +        return super.nextLong();
>> >>>>>>>> +    }
>> >>>>>>>> +
>> >>>>>>>>  /** {@inheritDoc} */
>> >>>>>>>>  @Override
>> >>>>>>>>  public String toString() {
>> >>>>>>>>
>> >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
>> > For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>>
>>