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, 9 Aug 2018 17:16:01 -0400, Rob Tompkins wrote:
I think we can make your changes Gilles, if we do a 2.0.

I know.

Would you
prefer to go that route?

Cure would be worse than the disease, for the reasons evoked.

Would it be possible to discuss *all* the arguments rather than
preemptively block the release as if a presumptive "JAR hell"
were the alpha and omega of reviewing?

If we go with 1.1 (other than via
deprecation), I think we’re stuck behind a bunch of -1 votes, and Id
rather not roll an RC to that audience.

We discussed a similar issue a few months ago; and we agreed
about some BC breaking, provided it is documented in the
release notes.

There are 5 projects using "Commons RNG", 2 of them depend on the
"commons-rng-sampling" module and none would be impacted by a BC
breaking release.  No JAR hell here.
If v1.1 is released, any new project will use it.  No JAR hell
there.
Please enlighten me on how JAR hell could arise from the proposed
changes.


Regards,
Gilles

I can work on the BC changes
or the version changes between now and next week.

-Rob

On Aug 9, 2018, at 10:49 AM, Gary Gregory <garydgregory@xxxxxxxxx> wrote:

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