[hibernate-dev] Let's avoid @hsearch.experimental javadoc tags in SPI

Wed Jun 7 10:24:38 EDT 2017

> Gunnar had a POC to create some great reports about private types
> leaking on public API.. that's something useful and possibly even ok
> to fail the build if that's violated - just we can't do it now as it
> would fail the build on our current API :) I was waiting for 6 to
> enable that, but maybe we should introduce it for new modules like ES
> already?

We've made this a part of the HV build since then:

    * https://github.com/hibernate/hibernate-validator/blob/master/pom.xml#L1146..L1168
    * https://github.com/hibernate/hibernate-validator/blob/master/jqassistant/rules.xml
    * http://in.relation.to/2017/01/31/preventing-leaky-apis-with-jqassistant/

This check runs once nightly so we're made aware of any implementation leakage.

One thing you could do is to set up the report in "warn mode" for the
existing modules and in "fail mode" for your new ones. And then of
course work towards addressing all warnings in the existing modules in
the course of working on HSEARCH 6.

2017-06-07 11:29 GMT+02:00 Sanne Grinovero <sanne at hibernate.org>:
> On 7 June 2017 at 09:19, Yoann Rodiere <yoann at hibernate.org> wrote:
>> Hi,
>>
>> Answers below.
>>
>> Yoann Rodière
>> Hibernate NoORM Team
>> yoann at hibernate.org
>>
>> On 6 June 2017 at 18:48, Sanne Grinovero <sanne at hibernate.org> wrote:
>>>
>>> == SPI have very long guarantees, anyway
>>>
>>> Considering that an SPI contract has relatively low SLAs I'd like to
>>> be way more "aggressive" in the removal of "this is experimental"
>>> notices.
>>>
>>> I'd also ask you all to consider not marking them as such, especially
>>> if the issue at hand feels properly sorted out.
>>>
>>> Most typically we'll have a notice like "This contract is currently
>>> under active development ".
>>>
>>> Anything which is marked as such and yet has not changed since the
>>> last minor release should no longer qualify as "active development",
>>> so we should clean it up and avoid scaring away potential users for no
>>> reason.
>>>
>>> WDYT?
>>
>>
>> Sure, it makes sense if we don't provide any long-term guarantee of backward
>> compatibility.
>>
>>>
>>> == What about API ?
>>>
>>> I am going to propose cleaning up the "experimental" tag from some
>>> selected APIs as well, yet in that case I don't think we should apply
>>> a similar reasoning as SPI (development activity) as time is not the
>>> right indicator.
>>>
>>> Some API has been marked experimental for a long time for good
>>> reasons: we might not have had the bandwidth (or interest) of
>>> finishing some related issue which could be essential for the feature
>>> to be actually used, and it might be problematic to fix such an half
>>> baked feature w/o being ale to make changes in the contract.
>>
>>
>> Yes. This basically means we don't have a solution for APIs, but... at least
>> we're aware of that :)
>
> Exactly
>
>>> == Reminder on changes
>>>
>>> Since we're on the subject... the API/SPI/impl classification is not
>>> black and white so bear in mind that any change we make will annoy
>>> someone.
>>>
>>> It's a tradeoff, but when you're making *any* change and have the
>>> opportunity to keep the contract backwards compatible, consider doing
>>> so. @Deprecated is a nice annotation.
>>> Of course this is not a reaction on any specific change; if any I'm
>>> just uncomfortable that I'm going to break a lot myself, and my own
>>> experience is that one can deal with some limited amount of
>>> compilation|compatibility issues but if we push too many "SPI & SPI-
>>> changes" in a single release people will get stuck on the older
>>> versions.
>>
>>
>> I agree with your analysis, but I'm not sure being aware of it and agreeing
>> that we shouldn't break too much will solve the problem. To me, a big part
>> of why we have a risk of breaking lots of things is that we currently don't
>> have the big picture until we write up the migration guide, just before we
>> release.
>> Maybe a way to solve the issue would be to write the migration guide
>> incrementally, with each PR we send?
>
> I like the idea of encouraging the discipline to "write it down" as
> part of the change work, like we do with tests and documentation, not
> least that makes one consider the impact better.
>
> Yet I loath the idea to make it more of an heavy process, especially
> if one has to coordinate PRs across different repositories - as our
> migration guide resides on the website.
>
> The ideal middleground would be to move the sources of the migration
> guide within the project's source control; the website should then
> incorporate it from our builds like it does with javadoc.
> An orthogonal issue is that javadoc is actually hosted on jboss.org
> while we wanted to move it into our website proper .. maybe we can fix
> both issues together when next hacking on the website build?
>
>
>> One way of enforcing this (reminding us to update the migration guide) would
>> be to have a tool check for API/SPI breaks on each build, and make the build
>> fail by default for any API/SPI change, requiring to at least put an
>> exclusion in a dedicated file to make it pass. Unless I'm wrong we currently
>> only do this in the engine/ORM modules. And actually I'm not sure the
>> current setup works, because I remember breaking SPIs since 5.7 and I never
>> had any build error relative to SPI breaks...
>> Anyway, if you agree with that, I can have a look at the tooling we may use.
>> I suspect other Hibernate projects may already have such tooling in place
>> (Hibernate Validator in particular).
>>
>>>
>>>
>>> We need to enable people to iterate in smaller milestones which are
>>> simpler to handle; if we're doing too much let's just make more
>>> intermediate minor releases.
>>
>>
>> If I remember correctly, that's exactly what we intended to do with 5.8,
>> except it's beginning to be a rather large intermediate minor itself :D
>
> Great summary of why I started this thread ;)
>
>> More seriously, yes, we should probably put less in each release, and in
>> particular have safeguards so that we don't realize there's too much in a
>> release when it's too late. But in order to do that, we should (in my
>> opinion) also have stricter monitoring of our work, and that's probably not
>> something we want, for various reasons we already discussed (one of them
>> being that we don't always know expansive and API/SPI-breaking a task will
>> be before we actually work on it).
>
> Right I'm very sceptical on that but if you feel the need to introduce
> something it's fine, but I would want such metrics to help identifying
> potential issues, not strictly prevent them.
> Gunnar had a POC to create some great reports about private types
> leaking on public API.. that's something useful and possibly even ok
> to fail the build if that's violated - just we can't do it now as it
> would fail the build on our current API :) I was waiting for 6 to
> enable that, but maybe we should introduce it for new modules like ES
> already?
>
> Remember that any single fix you apply will break something, it's
> always a trade-off:
>  - https://xkcd.com/1172/
>
> Fun story: I actually broke other projects by making our code more efficient..
>
> On a typically PR review when you move files I try to figure out if
> you really had to, but in 90% of cases I will prefer them to move to
> their new, better place as proposed and have us "move on" quickly
> without a debate on the subject. The alternative is that merging PRs
> would take twice as long and we can't afford that.
>
> The remaining 10% typically relates to me having a feeling it's going
> to create a mess for downstream projects like Infinispan... I see no
> better solution than checking out the Infinispan code and see for
> myself how hard it will be to adapt. Even so we can't do that for all
> PRs, not even all Beta releases, as building Infinispan sucks up half
> of your day's productivity.
> N.B. That pretty much defines the difference SPI vs impl: we don't
> want them to need to adapt in a micro release as they might need a
> quick bugfix from Hibernate Search without changing their own code -
> but in case of need we can actually amend their code when bumping the
> Search version so if it happens it's not the end of the world. That's
> true for Infinispan but also for any other downstream project, so no
> need to be too paranoid with strict automated checks.
>
> Thanks,
> Sanne
>
>
>>
>>
>>>
>>>
>>> Thanks!
>>> Sanne
>>> _______________________________________________
>>> hibernate-dev mailing list
>>> hibernate-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>>
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev