6:03 a.m.
Hi,
Before 4.4, when you add a @ContainedIn to a property without the
corresponding @IndexedEmbedded, they were still added and taken into
account in the dependency resolution which was quite useful to declare
dependencies between entities without having an explicit
@IndexedEmbedded which are useless in some of our cases.
Starting with 4.4, they are simply ignored.
I've spent some time to understand why and here it is:
https://github.com/hibernate/hibernate-search/blob/4.3/engine/src/main/ja...
-> you can see that the @Contained is added even if there is no
corresponding @IndexedEmbedded
In the new code, the @Contained annotation is added in
updateDepthProperties which is conditioned by the fact that there is a
corresponding @IndexedEmbedded:
https://github.com/hibernate/hibernate-search/blob/master/engine/src/main...
https://github.com/hibernate/hibernate-search/blob/4.3/engine/src/main/ja...
https://github.com/hibernate/hibernate-search/blob/master/engine/src/main...
I'm wondering if it's a desired change or just an overenthusiastic refactoring?
We used the old behavior in several projects so we would really like
to see it back if it is the latter.
And if it's the former, we would like to discuss it :).
Thanks for your feedback.
--
Guillaume
6:35 a.m.
FWIW, we tried to work around it by adding @IndexedEmbedded(depth = 0)
but as we also have @ContainedIn on transient methods, we can't work
around it totally.
As usual we are committed to work on a test case and fix once we agree
it should be fixed.
On Thu, Mar 27, 2014 at 11:03 AM, Guillaume Smet
<guillaume.smet(a)hibernate.org> wrote:
Hi,
Before 4.4, when you add a @ContainedIn to a property without the
corresponding @IndexedEmbedded, they were still added and taken into
account in the dependency resolution which was quite useful to declare
dependencies between entities without having an explicit
@IndexedEmbedded which are useless in some of our cases.
Starting with 4.4, they are simply ignored.
I've spent some time to understand why and here it is:
https://github.com/hibernate/hibernate-search/blob/4.3/engine/src/main/ja...
-> you can see that the @Contained is added even if there is no
corresponding @IndexedEmbedded
In the new code, the @Contained annotation is added in
updateDepthProperties which is conditioned by the fact that there is a
corresponding @IndexedEmbedded:
https://github.com/hibernate/hibernate-search/blob/master/engine/src/main...
https://github.com/hibernate/hibernate-search/blob/4.3/engine/src/main/ja...
https://github.com/hibernate/hibernate-search/blob/master/engine/src/main...
I'm wondering if it's a desired change or just an overenthusiastic refactoring?
We used the old behavior in several projects so we would really like
to see it back if it is the latter.
And if it's the former, we would like to discuss it :).
Thanks for your feedback.
--
Guillaume
7:51 a.m.
On 27 Jan 2014, at 11:03, Guillaume Smet <guillaume.smet(a)hibernate.org> wrote:
Before 4.4, when you add a @ContainedIn to a property without the
corresponding @IndexedEmbedded, they were still added and taken into
account in the dependency resolution which was quite useful to declare
dependencies between entities without having an explicit
@IndexedEmbedded which are useless in some of our cases.
What exactly do you mean with "taken into account in the dependency resolution”?
Maybe a full example would help here.
I'm wondering if it's a desired change or just an
overenthusiastic refactoring?
It might be. Still trying to understand what you are saying and the full implications.
In particular I am wondering where a @ContainedIn without the matching @indexedEmbedded
makes sense. Custom bridges come to mind in this case. Are you using a custom class or
field bridge in this case? If not, I don’t understand what purpose the @ContainedIn would
fulfil.
—Hardy
7:52 a.m.
On 27 Jan 2014, at 11:35, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
FWIW, we tried to work around it by adding @IndexedEmbedded(depth =
0)
but as we also have @ContainedIn on transient methods, we can't work
around it totally.
As usual we are committed to work on a test case and fix once we agree
it should be fixed.
I think an example/test would be awesome. Then we have some concrete example
to base our discussion upon.
—Hardy
9:46 a.m.
Hi Hardy,
On Thu, Mar 27, 2014 at 12:52 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
I think an example/test would be awesome. Then we have some concrete
example
to base our discussion upon.
I don't think a test would be useful to understand what we do. Here
are a few additional information about our business case.
The fact is that we have complex business rules which requires us to
reindex entities when we change/index one even if they don't have an
@IndexedEmbedded relation.
You're right about having specific fieldbridges but we also have the
case when we want to index the results of a transient method on a
dependency of the object.
In one of our (many) examples of this usage, we have:
ProductModel
@ContainedIn
Set<ProductArticle> articles;
ProductArticle doesn't have an @IndexedEmbedded annotation on its
ProductModel field because we don't use this feature to index it. But
when we reindex a ProductModel, we need to reindex the articles.
Moreover, an article might have a ShootingBrief and when we change the
ProductModel, we also want to reindex the ShootingBriefs of the
ProductArticles as they have a field which depends on a ProductModel
property.
This field is the result of a transient method. Not an @IndexedEmbedded thing.
Using @ContainedIn as we do allows us to build a dependency graph of
indexing. And this dependency graph exists even if we don't use
@IndexedEmbedded but other Search features (FieldBridges, @Field on a
transient method...).
Note that it worked perfectly until 4.4.
I don't know if it's more clear with these information. Feel free to
ping me on IRC to discuss it further if needed.
--
Guillaume
5:52 a.m.
Hardy,
This one is really a blocker for us as we have to put something into
production really soon with 4.4.2 and we can't do it atm. I take the
liberty to open a JIRA issue so that I can start working on a PR with
a test showing what we do and a fix.
While it looks like it was not an intented feature at the time, it's a
very useful one so it would be nice to make it work again. I don't
think it has any drawbacks to come back to the preexisting behavior.
I understand there probably won't be a release of 4.4.x soon (if any)
to fix this but, once we agree on the fix, I can deploy something on
our internal repo.
--
Guillaume
On Thu, Mar 27, 2014 at 2:46 PM, Guillaume Smet
<guillaume.smet(a)gmail.com> wrote:
Hi Hardy,
On Thu, Mar 27, 2014 at 12:52 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
> I think an example/test would be awesome. Then we have some concrete example
> to base our discussion upon.
I don't think a test would be useful to understand what we do. Here
are a few additional information about our business case.
The fact is that we have complex business rules which requires us to
reindex entities when we change/index one even if they don't have an
@IndexedEmbedded relation.
You're right about having specific fieldbridges but we also have the
case when we want to index the results of a transient method on a
dependency of the object.
In one of our (many) examples of this usage, we have:
ProductModel
@ContainedIn
Set<ProductArticle> articles;
ProductArticle doesn't have an @IndexedEmbedded annotation on its
ProductModel field because we don't use this feature to index it. But
when we reindex a ProductModel, we need to reindex the articles.
Moreover, an article might have a ShootingBrief and when we change the
ProductModel, we also want to reindex the ShootingBriefs of the
ProductArticles as they have a field which depends on a ProductModel
property.
This field is the result of a transient method. Not an @IndexedEmbedded thing.
Using @ContainedIn as we do allows us to build a dependency graph of
indexing. And this dependency graph exists even if we don't use
@IndexedEmbedded but other Search features (FieldBridges, @Field on a
transient method...).
Note that it worked perfectly until 4.4.
I don't know if it's more clear with these information. Feel free to
ping me on IRC to discuss it further if needed.
--
Guillaume
6:35 a.m.
On 27 Jan 2014, at 14:46, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
I don't think a test would be useful to understand what we do.
Here
are a few additional information about our business case.
Not sure. I think it always helps to see the whole picture.
The fact is that we have complex business rules which requires us to
reindex entities when we change/index one even if they don't have an
@IndexedEmbedded relation.
It seems to me you were relying on some side effect of ContainedIn.
AFAIK @ContainedIn has always been intended to be the counterpart to @IndexedEmbedded,
not as a general reindexing trigger. That’s also what the docs imply imo.
For me @ContainedIn used in an entity X means, that the entity Y owning the other side of
the associations needs
reindexing when X changes, because because the index of Y contains embedded fields from
X.
You seem to see @ContainedIn as a general trigger to reindex the other side of the
association
no matter what.
I kind of see your use case, but if we go down your path, I think we also need to clarify
the intention
of @ContainedIn
In one of our (many) examples of this usage, we have:
ProductModel
@ContainedIn
Set<ProductArticle> articles;
ProductArticle doesn't have an @IndexedEmbedded annotation on its
ProductModel field because we don't use this feature to index it. But
when we reindex a ProductModel, we need to reindex the articles.
So how does get information of the changed ProductModel into the ProductArticle?
Moreover, an article might have a ShootingBrief and when we change
the
ProductModel, we also want to reindex the ShootingBriefs of the
ProductArticles as they have a field which depends on a ProductModel
property.
Hmm, ok.
This field is the result of a transient method. Not an
@IndexedEmbedded thing.
Using @ContainedIn as we do allows us to build a dependency graph of
indexing. And this dependency graph exists even if we don't use
@IndexedEmbedded but other Search features (FieldBridges, @Field on a
transient method…).
Ok
—Hardy
6:44 a.m.
On 28 Jan 2014, at 10:52, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
This one is really a blocker for us as we have to put something into
production really soon with 4.4.2 and we can't do it atm. I take the
liberty to open a JIRA issue so that I can start working on a PR with
a test showing what we do and a fix.
Sure
While it looks like it was not an intented feature at the time,
it's a
very useful one so it would be nice to make it work again.
As said, I don’t think it was and I think I understand your use case by now.
There are two things I am wondering about atm:
- will this change effect the indexing optimisation code we have in place which
tries to skip indexing in cases were the changed entity properties don’t effect
the index
- how do we clarify the behaviour of ContainedIn in the docs (it is not just the
counterpart of @IndexedEmbedded, but a standalone annotation/feature)
I don't
think it has any drawbacks to come back to the preexisting behaviour.
Only thing which comes to mind is this indexing optimisation part, but it might not
even be effected.
I understand there probably won't be a release of 4.4.x soon (if
any)
to fix this but, once we agree on the fix, I can deploy something on
our internal repo.
Ok.
—Hardy
7:16 a.m.
We could use either a different annotation or a flag
@ContainedIn(someVerySmartName=SomeVerySmartValue)
On Fri 2014-03-28 11:44, Hardy Ferentschik wrote:
On 28 Jan 2014, at 10:52, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
> This one is really a blocker for us as we have to put something into
> production really soon with 4.4.2 and we can't do it atm. I take the
> liberty to open a JIRA issue so that I can start working on a PR with
> a test showing what we do and a fix.
Sure
> While it looks like it was not an intented feature at the time, it's a
> very useful one so it would be nice to make it work again.
As said, I don’t think it was and I think I understand your use case by now.
There are two things I am wondering about atm:
- will this change effect the indexing optimisation code we have in place which
tries to skip indexing in cases were the changed entity properties don’t effect
the index
- how do we clarify the behaviour of ContainedIn in the docs (it is not just the
counterpart of @IndexedEmbedded, but a standalone annotation/feature)
> I don't
> think it has any drawbacks to come back to the preexisting behaviour.
Only thing which comes to mind is this indexing optimisation part, but it might not
even be effected.
> I understand there probably won't be a release of 4.4.x soon (if any)
> to fix this but, once we agree on the fix, I can deploy something on
> our internal repo.
Ok.
—Hardy
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
7:37 a.m.
On 28 Jan 2014, at 12:16, Emmanuel Bernard <emmanuel(a)hibernate.org> wrote:
We could use either a different annotation or a flag
@ContainedIn(someVerySmartName=SomeVerySmartValue)
I was also wondering whether a whole new annotation would make sense, but nothing
intuitive
came to mind right away. I guess @ContainedIn cold work. It is just that we need to make
clear
what it means and how it behaves.
—Hardy
7:56 a.m.
On 28 March 2014 11:37, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
On 28 Jan 2014, at 12:16, Emmanuel Bernard <emmanuel(a)hibernate.org> wrote:
> We could use either a different annotation or a flag
>
> @ContainedIn(someVerySmartName=SomeVerySmartValue)
I was also wondering whether a whole new annotation would make sense, but nothing
intuitive
came to mind right away. I guess @ContainedIn cold work. It is just that we need to make
clear
what it means and how it behaves.
+1
Since 4.4 is the last one supporting a wide an popular range of
Hibernate versions and application server versions, it's totally
possible that we'll be making more bugifx releases if the need shows
up.
However - I might not have fully grasped this yet - but I'm thinking
that this is a feature request and not a bugfix that should be hastily
applied on 4.4.
Guillaume, I suspect you might be a happier user if you would keep
using a more bleeding edge version, so to catch any such thing earlier
rather than the day before you go in production. I can only promise
you that the migration to 5.0 will be a nightmare if you don't follow
along in small iterations :-(
Sanne
—Hardy
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
8:28 a.m.
Hi Sanne,
On Fri, Mar 28, 2014 at 12:56 PM, Sanne Grinovero <sanne(a)hibernate.org> wrote:
However - I might not have fully grasped this yet - but I'm
thinking
that this is a feature request and not a bugfix that should be hastily
applied on 4.4.
It's not a feature request. 4.4 changed this behavior. It was working
well with 4.3 (and every other versions before that).
Guillaume, I suspect you might be a happier user if you would keep
using a more bleeding edge version, so to catch any such thing earlier
rather than the day before you go in production. I can only promise
you that the migration to 5.0 will be a nightmare if you don't follow
along in small iterations :-(
I don't think we can be more bleeding edge than us. We upgrade our
core framework to new versions as soon as they are released (we coded
https://www.artifact-listener.org/ for this purpose). That's why we
spend a considerable amount of time doing QA on the components we use
- you see a part of this work on ORM + Search but we also do it for
other components. You might have noticed that we are often the first
company out there to catch regressions.
When we have the ability to dedicate cycles to it, we also test CR.
But... we develop a lot of applications for a lot of customers and
they have different lifecycles: we can upgrade some easily and for
others we have to wait for the right opportunity to do it.
All in all, we have the following policy:
- all the applications in development: they use the latest versions of
each components, except if we catch a regression when upgrading. If so
we try to help fix it for the next version and we upgrade ASAP;
- the applications in production are updated regularly for minor
changes; If the changes are big, we wait for our customers to ask
changes on the app and we do it then.
The fact is that we caught this one while upgrading an app which was
using 4.3.0.Final (which is not that old) but I think we have other
applications already upgraded to 4.4.2 which have that bug, it's just
that we haven't caught it when we upgraded.
FWIW, we couldn't upgrade to 4.4.x before 4.4.2.Final because of a few
regressions we reported and helped getting fixed (I have at least
these ones in mind: HSEARCH-1442 and HSEARCH-1462 but IIRC we had
others).
(Sorry for the noise but I thought it was a good opportunity to
explain how we work, considering we're involved in this community for
quite a long time now)
--
Guillaume
9:32 a.m.
Hi again,
I posted a pull request for the 4.5 branch:
https://github.com/hibernate/hibernate-search/pull/590 . Comments
welcome!
I included a rather large test case I have extracted from our app.
It's simplified but I think you should get the idea by looking at it.
All tests passes.
FWIW, I find the new way to do it more consistent with the names of
the methods as updateContainedInMaxDepths now does exactly what its
name says.
It would be nice if it could also be backported to 4.4.x.
Glad to discuss further enhancements if needed but I think the old
behavior should be brought back before considering any further
enhancements/features.
--
Guillaume
On Fri, Mar 28, 2014 at 1:28 PM, Guillaume Smet
<guillaume.smet(a)gmail.com> wrote:
Hi Sanne,
On Fri, Mar 28, 2014 at 12:56 PM, Sanne Grinovero <sanne(a)hibernate.org> wrote:
> However - I might not have fully grasped this yet - but I'm thinking
> that this is a feature request and not a bugfix that should be hastily
> applied on 4.4.
It's not a feature request. 4.4 changed this behavior. It was working
well with 4.3 (and every other versions before that).
> Guillaume, I suspect you might be a happier user if you would keep
> using a more bleeding edge version, so to catch any such thing earlier
> rather than the day before you go in production. I can only promise
> you that the migration to 5.0 will be a nightmare if you don't follow
> along in small iterations :-(
I don't think we can be more bleeding edge than us. We upgrade our
core framework to new versions as soon as they are released (we coded
https://www.artifact-listener.org/ for this purpose). That's why we
spend a considerable amount of time doing QA on the components we use
- you see a part of this work on ORM + Search but we also do it for
other components. You might have noticed that we are often the first
company out there to catch regressions.
When we have the ability to dedicate cycles to it, we also test CR.
But... we develop a lot of applications for a lot of customers and
they have different lifecycles: we can upgrade some easily and for
others we have to wait for the right opportunity to do it.
All in all, we have the following policy:
- all the applications in development: they use the latest versions of
each components, except if we catch a regression when upgrading. If so
we try to help fix it for the next version and we upgrade ASAP;
- the applications in production are updated regularly for minor
changes; If the changes are big, we wait for our customers to ask
changes on the app and we do it then.
The fact is that we caught this one while upgrading an app which was
using 4.3.0.Final (which is not that old) but I think we have other
applications already upgraded to 4.4.2 which have that bug, it's just
that we haven't caught it when we upgraded.
FWIW, we couldn't upgrade to 4.4.x before 4.4.2.Final because of a few
regressions we reported and helped getting fixed (I have at least
these ones in mind: HSEARCH-1442 and HSEARCH-1462 but IIRC we had
others).
(Sorry for the noise but I thought it was a good opportunity to
explain how we work, considering we're involved in this community for
quite a long time now)
--
Guillaume
8:24 a.m.
Hi,
I think we dropped the ball on this one. I basically had a look at Guillaume’s pull
request.
His analysis was correct and his proposed patch brings back the old pre 4.4 behaviour
with minimal changes.
There is still the question of the indented use cases of @ContainedIn. As discussed in
this thread
afaik the indent was as a sole counterpart to @IndexedEmbedded. The implementation
(probably
as a side effect) made it also work when @IndexedEmbedded was not used. This was changed
with
the metamodel refactoring where contained in was indeed treated as counterpart of indexed
embedded.
I think we should go ahead and apply Guillaume’s pull request for 4.4 and 4.5, basically
reinstating
old 4.x behaviour, side effect or not.
The next question is then what to do in Search 5. I think we can just carry forward the
change/patch.
I don’t think that @ContainedIn needs another attribute or that we should introduce a
whole new annotation.
@ContainedIn seems quite natural provided we documented it intend and behaviour. Basically
all what @ContainedIn
is saying, is that here we have a reference to another entity which needs reindexing as
well when the current
entity gets reindexed. Whether the inclusions happens via @IndexEmbedded or via a custom
bridge is irrelevant.
Thoughts?
—Hardy
P.S. Sanne, are you planning 4.4 and 4.5 releases as well? Just asking due to the recent
back ports. If so, we should
include this patch as well.
On 28 Jan 2014, at 14:32, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
Hi again,
I posted a pull request for the 4.5 branch:
https://github.com/hibernate/hibernate-search/pull/590 . Comments
welcome!
I included a rather large test case I have extracted from our app.
It's simplified but I think you should get the idea by looking at it.
All tests passes.
FWIW, I find the new way to do it more consistent with the names of
the methods as updateContainedInMaxDepths now does exactly what its
name says.
It would be nice if it could also be backported to 4.4.x.
Glad to discuss further enhancements if needed but I think the old
behavior should be brought back before considering any further
enhancements/features.
--
Guillaume
On Fri, Mar 28, 2014 at 1:28 PM, Guillaume Smet
<guillaume.smet(a)gmail.com> wrote:
> Hi Sanne,
>
> On Fri, Mar 28, 2014 at 12:56 PM, Sanne Grinovero <sanne(a)hibernate.org> wrote:
>> However - I might not have fully grasped this yet - but I'm thinking
>> that this is a feature request and not a bugfix that should be hastily
>> applied on 4.4.
>
> It's not a feature request. 4.4 changed this behavior. It was working
> well with 4.3 (and every other versions before that).
>
>> Guillaume, I suspect you might be a happier user if you would keep
>> using a more bleeding edge version, so to catch any such thing earlier
>> rather than the day before you go in production. I can only promise
>> you that the migration to 5.0 will be a nightmare if you don't follow
>> along in small iterations :-(
>
> I don't think we can be more bleeding edge than us. We upgrade our
> core framework to new versions as soon as they are released (we coded
> https://www.artifact-listener.org/ for this purpose). That's why we
> spend a considerable amount of time doing QA on the components we use
> - you see a part of this work on ORM + Search but we also do it for
> other components. You might have noticed that we are often the first
> company out there to catch regressions.
>
> When we have the ability to dedicate cycles to it, we also test CR.
>
> But... we develop a lot of applications for a lot of customers and
> they have different lifecycles: we can upgrade some easily and for
> others we have to wait for the right opportunity to do it.
>
> All in all, we have the following policy:
> - all the applications in development: they use the latest versions of
> each components, except if we catch a regression when upgrading. If so
> we try to help fix it for the next version and we upgrade ASAP;
> - the applications in production are updated regularly for minor
> changes; If the changes are big, we wait for our customers to ask
> changes on the app and we do it then.
>
> The fact is that we caught this one while upgrading an app which was
> using 4.3.0.Final (which is not that old) but I think we have other
> applications already upgraded to 4.4.2 which have that bug, it's just
> that we haven't caught it when we upgraded.
>
> FWIW, we couldn't upgrade to 4.4.x before 4.4.2.Final because of a few
> regressions we reported and helped getting fixed (I have at least
> these ones in mind: HSEARCH-1442 and HSEARCH-1462 but IIRC we had
> others).
>
> (Sorry for the noise but I thought it was a good opportunity to
> explain how we work, considering we're involved in this community for
> quite a long time now)
>
> --
> Guillaume
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
9:28 a.m.
On 3 April 2014 13:24, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
Hi,
I think we dropped the ball on this one. I basically had a look at Guillaume’s pull
request.
His analysis was correct and his proposed patch brings back the old pre 4.4 behaviour
with minimal changes.
Ok that sounds good, I couldn't look at the PR yet.. should I not trust you ? :)
There is still the question of the indented use cases of @ContainedIn. As discussed in
this thread
afaik the indent was as a sole counterpart to @IndexedEmbedded. The implementation
(probably
as a side effect) made it also work when @IndexedEmbedded was not used. This was changed
with
the metamodel refactoring where contained in was indeed treated as counterpart of indexed
embedded.
I think we should go ahead and apply Guillaume’s pull request for 4.4 and 4.5, basically
reinstating
old 4.x behaviour, side effect or not.
+1
The next question is then what to do in Search 5. I think we can just carry forward the
change/patch.
Yes let's keep them in sync for now
I don’t think that @ContainedIn needs another attribute or that we
should introduce a whole new annotation.
@ContainedIn seems quite natural provided we documented it intend and behaviour.
Basically all what @ContainedIn
is saying, is that here we have a reference to another entity which needs reindexing as
well when the current
entity gets reindexed. Whether the inclusions happens via @IndexEmbedded or via a custom
bridge is irrelevant.
Thoughts?
I agree with you, but in insight if this new "meaning" would have been
explicit on this annotation from the beginning of time, maybe it would
have had a different name?
I don't think the name is appropriate for this more extended meaning;
probably not a big problem but we can decide on that after it's fixed.
—Hardy
P.S. Sanne, are you planning 4.4 and 4.5 releases as well? Just asking due to the recent
back ports. If so, we should
include this patch as well.
Some of the backports I did because I indeed want to maintain these
older branches, although to keep them stable I'd not be too eager on
backports.
We don't have dates defined, but we can certainly release these when
there are enough good reasons to: for example if Guillaume needs it.
Sanne
On 28 Jan 2014, at 14:32, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
> Hi again,
>
> I posted a pull request for the 4.5 branch:
> https://github.com/hibernate/hibernate-search/pull/590 . Comments
> welcome!
>
> I included a rather large test case I have extracted from our app.
> It's simplified but I think you should get the idea by looking at it.
>
> All tests passes.
>
> FWIW, I find the new way to do it more consistent with the names of
> the methods as updateContainedInMaxDepths now does exactly what its
> name says.
>
> It would be nice if it could also be backported to 4.4.x.
>
> Glad to discuss further enhancements if needed but I think the old
> behavior should be brought back before considering any further
> enhancements/features.
>
> --
> Guillaume
>
> On Fri, Mar 28, 2014 at 1:28 PM, Guillaume Smet
> <guillaume.smet(a)gmail.com> wrote:
>> Hi Sanne,
>>
>> On Fri, Mar 28, 2014 at 12:56 PM, Sanne Grinovero <sanne(a)hibernate.org>
wrote:
>>> However - I might not have fully grasped this yet - but I'm thinking
>>> that this is a feature request and not a bugfix that should be hastily
>>> applied on 4.4.
>>
>> It's not a feature request. 4.4 changed this behavior. It was working
>> well with 4.3 (and every other versions before that).
>>
>>> Guillaume, I suspect you might be a happier user if you would keep
>>> using a more bleeding edge version, so to catch any such thing earlier
>>> rather than the day before you go in production. I can only promise
>>> you that the migration to 5.0 will be a nightmare if you don't follow
>>> along in small iterations :-(
>>
>> I don't think we can be more bleeding edge than us. We upgrade our
>> core framework to new versions as soon as they are released (we coded
>> https://www.artifact-listener.org/ for this purpose). That's why we
>> spend a considerable amount of time doing QA on the components we use
>> - you see a part of this work on ORM + Search but we also do it for
>> other components. You might have noticed that we are often the first
>> company out there to catch regressions.
>>
>> When we have the ability to dedicate cycles to it, we also test CR.
>>
>> But... we develop a lot of applications for a lot of customers and
>> they have different lifecycles: we can upgrade some easily and for
>> others we have to wait for the right opportunity to do it.
>>
>> All in all, we have the following policy:
>> - all the applications in development: they use the latest versions of
>> each components, except if we catch a regression when upgrading. If so
>> we try to help fix it for the next version and we upgrade ASAP;
>> - the applications in production are updated regularly for minor
>> changes; If the changes are big, we wait for our customers to ask
>> changes on the app and we do it then.
>>
>> The fact is that we caught this one while upgrading an app which was
>> using 4.3.0.Final (which is not that old) but I think we have other
>> applications already upgraded to 4.4.2 which have that bug, it's just
>> that we haven't caught it when we upgraded.
>>
>> FWIW, we couldn't upgrade to 4.4.x before 4.4.2.Final because of a few
>> regressions we reported and helped getting fixed (I have at least
>> these ones in mind: HSEARCH-1442 and HSEARCH-1462 but IIRC we had
>> others).
>>
>> (Sorry for the noise but I thought it was a good opportunity to
>> explain how we work, considering we're involved in this community for
>> quite a long time now)
>>
>> --
>> Guillaume
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
10:07 a.m.
Hi Sanne,
On Thu, Apr 3, 2014 at 3:28 PM, Sanne Grinovero <sanne(a)hibernate.org> wrote:
We don't have dates defined, but we can certainly release these
when
there are enough good reasons to: for example if Guillaume needs it.
It would be nice to have an official release once we have a fix for
https://hibernate.atlassian.net/browse/HSEARCH-1583 but I don't think
it's a good idea to release something before that as it's a kinda
annoying one too.
We have moved our most advanced/tested application in terms of
indexing features to 4.4 so I think we probably won't find any more
regression in it as for the features we use (we can't move this app to
ORM 4.3 for now due to other big changes in our core framework).
We have a couple of applications running on ORM 4.3 + Search 4.5 and
we don't have any problem apart from the ones we already opened.
--
Guillaume
2:18 p.m.
On 3 Jan 2014, at 15:28, Sanne Grinovero <sanne(a)hibernate.org> wrote:
> I think we dropped the ball on this one. I basically had a look
at Guillaume’s pull request.
> His analysis was correct and his proposed patch brings back the old pre 4.4
behaviour
> with minimal changes.
Ok that sounds good, I couldn't look at the PR yet.. should I not trust you ? :)
You definitely, should ;-) I just felt that we kind of left the discussion open ended.
> There is still the question of the indented use cases of
@ContainedIn. As discussed in this thread
> afaik the indent was as a sole counterpart to @IndexedEmbedded. The implementation
(probably
> as a side effect) made it also work when @IndexedEmbedded was not used. This was
changed with
> the metamodel refactoring where contained in was indeed treated as counterpart of
indexed embedded.
>
> I think we should go ahead and apply Guillaume’s pull request for 4.4 and 4.5,
basically reinstating
> old 4.x behaviour, side effect or not.
+1
Ok then
> The next question is then what to do in Search 5. I think we can
just carry forward the change/patch.
Yes let's keep them in sync for now
> I don’t think that @ContainedIn needs another attribute or that we should introduce a
whole new annotation.
> @ContainedIn seems quite natural provided we documented it intend and behaviour.
Basically all what @ContainedIn
> is saying, is that here we have a reference to another entity which needs reindexing
as well when the current
> entity gets reindexed. Whether the inclusions happens via @IndexEmbedded or via a
custom bridge is irrelevant.
>
> Thoughts?
I agree with you, but in insight if this new "meaning" would have been
explicit on this annotation from the beginning of time, maybe it would
have had a different name?
Maybe. But what? @ContainedIn kind of fits.
I don't think the name is appropriate for this more extended
meaning;
probably not a big problem but we can decide on that after it's fixed.
Sure.
We don't have dates defined, but we can certainly release these
when
there are enough good reasons to: for example if Guillaume needs it.
ok
3675
days inactive
3682
days old
16 comments
5 participants
participants (5)
-
Emmanuel Bernard -
Guillaume Smet -
Guillaume Smet -
Hardy Ferentschik -
Sanne Grinovero