You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@karaf.apache.org by Daniel Estermann <es...@apache.org> on 2019/03/05 17:19:38 UTC
FeaturesProcessorImpl improvement of bundle override
Hi everybody,
we have the following override.properties:
mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
which are supposed to do the following replacements:
portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
and
portal-backend-sdk/2.68.3/jar/karaf-migration ->
portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
But the method
org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
<https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225>
replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
classified artifact gets replaced with an artifact without classifier. This
happens because the implementation of
org.apache.karaf.features.LocationPattern
<https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151>
allows matching of classified URI by a non-classified pattern. The
LocationPatternTest indicates that this is an intentional behavior: see
line 112
<https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112>,
115
<https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115>,
116
<https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116>
.
I can understand why LocationPattern is implemented like that. But then my
guess is that FeaturesProcessorImpl is incorrect. By the way an interesting
comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
work for us so looks like now the time has come to check it. I think what
is crucial there, is that we shouldn't rely on the order of overrides, but
rather on their quality. I.e. not the first/last match is appropriate, but
the best one. I propose to collect candidates to match and then determine
the best one using simply the length as criterion.
My proposal is already implemented in our fork and tested on my local
system (for now). Here you can see it:
https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
I would appreciate to receive any feedback on this.
Regards,
Daniel
Re: FeaturesProcessorImpl improvement of bundle override
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
Thanks, it's in my bucket. I will integrate for 4.2.4 !
Regards
JB
On 06/03/2019 15:36, Daniel Estermann wrote:
> Thanks guys for your great feedback! As requested I created everything we need to make it official:
>
> The issue https://issues.apache.org/jira/browse/KARAF-6183
> And the PR https://github.com/apache/karaf/pull/772
>
> Cheers
> Daniel
>
> On 2019/03/06 06:32:18, Grzegorz Grzybek <gr...@gmail.com> wrote:
>> Hello
>>
>> Looks like I was anticipating such problem by adding "TOCHECK: last rule
>> wins - no break!!!" :)
>> The diff looks good - if all existing tests pass, I think we can merge it.
>> Could you please create KARAF jira issue for this?
>>
>> best regards
>> Grzegorz Grzybek
>>
>> wt., 5 mar 2019 o 18:27 Jean-Baptiste Onofré <jb...@nanthrax.net> napisał(a):
>>
>>> Thanks for sharing Daniel.
>>>
>>> I gonna take a look.
>>>
>>> Regards
>>> JB
>>>
>>> On 05/03/2019 18:19, Daniel Estermann wrote:
>>>> Hi everybody,
>>>>
>>>> we have the following override.properties:
>>>>
>>>>
>>> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
>>>>
>>> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
>>>>
>>>> which are supposed to do the following replacements:
>>>>
>>>> portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
>>>>
>>>> and
>>>>
>>>> portal-backend-sdk/2.68.3/jar/karaf-migration ->
>>>> portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
>>>>
>>>> But the method
>>>>
>>> org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
>>>> <
>>> https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225
>>>>
>>>> replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
>>>> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
>>>> classified artifact gets replaced with an artifact without classifier.
>>> This
>>>> happens because the implementation of
>>>> org.apache.karaf.features.LocationPattern
>>>> <
>>> https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151
>>>>
>>>> allows matching of classified URI by a non-classified pattern. The
>>>> LocationPatternTest indicates that this is an intentional behavior: see
>>>> line 112
>>>> <
>>> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112
>>>> ,
>>>> 115
>>>> <
>>> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115
>>>> ,
>>>> 116
>>>> <
>>> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116
>>>>
>>>> .
>>>>
>>>> I can understand why LocationPattern is implemented like that. But then
>>> my
>>>> guess is that FeaturesProcessorImpl is incorrect. By the way an
>>> interesting
>>>> comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
>>>> work for us so looks like now the time has come to check it. I think what
>>>> is crucial there, is that we shouldn't rely on the order of overrides,
>>> but
>>>> rather on their quality. I.e. not the first/last match is appropriate,
>>> but
>>>> the best one. I propose to collect candidates to match and then determine
>>>> the best one using simply the length as criterion.
>>>>
>>>> My proposal is already implemented in our fork and tested on my local
>>>> system (for now). Here you can see it:
>>>>
>>> https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
>>>>
>>>> I would appreciate to receive any feedback on this.
>>>>
>>>> Regards,
>>>> Daniel
>>>>
>>>
>>
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com
Re: FeaturesProcessorImpl improvement of bundle override
Posted by Daniel Estermann <es...@apache.org>.
Thanks guys for your great feedback! As requested I created everything we need to make it official:
The issue https://issues.apache.org/jira/browse/KARAF-6183
And the PR https://github.com/apache/karaf/pull/772
Cheers
Daniel
On 2019/03/06 06:32:18, Grzegorz Grzybek <gr...@gmail.com> wrote:
> Hello
>
> Looks like I was anticipating such problem by adding "TOCHECK: last rule
> wins - no break!!!" :)
> The diff looks good - if all existing tests pass, I think we can merge it.
> Could you please create KARAF jira issue for this?
>
> best regards
> Grzegorz Grzybek
>
> wt., 5 mar 2019 o 18:27 Jean-Baptiste Onofré <jb...@nanthrax.net> napisał(a):
>
> > Thanks for sharing Daniel.
> >
> > I gonna take a look.
> >
> > Regards
> > JB
> >
> > On 05/03/2019 18:19, Daniel Estermann wrote:
> > > Hi everybody,
> > >
> > > we have the following override.properties:
> > >
> > >
> > mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
> > >
> > mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
> > >
> > > which are supposed to do the following replacements:
> > >
> > > portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
> > >
> > > and
> > >
> > > portal-backend-sdk/2.68.3/jar/karaf-migration ->
> > > portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
> > >
> > > But the method
> > >
> > org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
> > > <
> > https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225
> > >
> > > replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
> > > mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
> > > classified artifact gets replaced with an artifact without classifier.
> > This
> > > happens because the implementation of
> > > org.apache.karaf.features.LocationPattern
> > > <
> > https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151
> > >
> > > allows matching of classified URI by a non-classified pattern. The
> > > LocationPatternTest indicates that this is an intentional behavior: see
> > > line 112
> > > <
> > https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112
> > >,
> > > 115
> > > <
> > https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115
> > >,
> > > 116
> > > <
> > https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116
> > >
> > > .
> > >
> > > I can understand why LocationPattern is implemented like that. But then
> > my
> > > guess is that FeaturesProcessorImpl is incorrect. By the way an
> > interesting
> > > comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
> > > work for us so looks like now the time has come to check it. I think what
> > > is crucial there, is that we shouldn't rely on the order of overrides,
> > but
> > > rather on their quality. I.e. not the first/last match is appropriate,
> > but
> > > the best one. I propose to collect candidates to match and then determine
> > > the best one using simply the length as criterion.
> > >
> > > My proposal is already implemented in our fork and tested on my local
> > > system (for now). Here you can see it:
> > >
> > https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
> > >
> > > I would appreciate to receive any feedback on this.
> > >
> > > Regards,
> > > Daniel
> > >
> >
>
Re: FeaturesProcessorImpl improvement of bundle override
Posted by Grzegorz Grzybek <gr...@gmail.com>.
Hello
Looks like I was anticipating such problem by adding "TOCHECK: last rule
wins - no break!!!" :)
The diff looks good - if all existing tests pass, I think we can merge it.
Could you please create KARAF jira issue for this?
best regards
Grzegorz Grzybek
wt., 5 mar 2019 o 18:27 Jean-Baptiste Onofré <jb...@nanthrax.net> napisał(a):
> Thanks for sharing Daniel.
>
> I gonna take a look.
>
> Regards
> JB
>
> On 05/03/2019 18:19, Daniel Estermann wrote:
> > Hi everybody,
> >
> > we have the following override.properties:
> >
> >
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
> >
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
> >
> > which are supposed to do the following replacements:
> >
> > portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
> >
> > and
> >
> > portal-backend-sdk/2.68.3/jar/karaf-migration ->
> > portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
> >
> > But the method
> >
> org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
> > <
> https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225
> >
> > replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
> > mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
> > classified artifact gets replaced with an artifact without classifier.
> This
> > happens because the implementation of
> > org.apache.karaf.features.LocationPattern
> > <
> https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151
> >
> > allows matching of classified URI by a non-classified pattern. The
> > LocationPatternTest indicates that this is an intentional behavior: see
> > line 112
> > <
> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112
> >,
> > 115
> > <
> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115
> >,
> > 116
> > <
> https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116
> >
> > .
> >
> > I can understand why LocationPattern is implemented like that. But then
> my
> > guess is that FeaturesProcessorImpl is incorrect. By the way an
> interesting
> > comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
> > work for us so looks like now the time has come to check it. I think what
> > is crucial there, is that we shouldn't rely on the order of overrides,
> but
> > rather on their quality. I.e. not the first/last match is appropriate,
> but
> > the best one. I propose to collect candidates to match and then determine
> > the best one using simply the length as criterion.
> >
> > My proposal is already implemented in our fork and tested on my local
> > system (for now). Here you can see it:
> >
> https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
> >
> > I would appreciate to receive any feedback on this.
> >
> > Regards,
> > Daniel
> >
>
Re: FeaturesProcessorImpl improvement of bundle override
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
Thanks for sharing Daniel.
I gonna take a look.
Regards
JB
On 05/03/2019 18:19, Daniel Estermann wrote:
> Hi everybody,
>
> we have the following override.properties:
>
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
>
> which are supposed to do the following replacements:
>
> portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
>
> and
>
> portal-backend-sdk/2.68.3/jar/karaf-migration ->
> portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
>
> But the method
> org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
> <https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225>
> replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
> classified artifact gets replaced with an artifact without classifier. This
> happens because the implementation of
> org.apache.karaf.features.LocationPattern
> <https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151>
> allows matching of classified URI by a non-classified pattern. The
> LocationPatternTest indicates that this is an intentional behavior: see
> line 112
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112>,
> 115
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115>,
> 116
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116>
> .
>
> I can understand why LocationPattern is implemented like that. But then my
> guess is that FeaturesProcessorImpl is incorrect. By the way an interesting
> comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
> work for us so looks like now the time has come to check it. I think what
> is crucial there, is that we shouldn't rely on the order of overrides, but
> rather on their quality. I.e. not the first/last match is appropriate, but
> the best one. I propose to collect candidates to match and then determine
> the best one using simply the length as criterion.
>
> My proposal is already implemented in our fork and tested on my local
> system (for now). Here you can see it:
> https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
>
> I would appreciate to receive any feedback on this.
>
> Regards,
> Daniel
>
Re: FeaturesProcessorImpl improvement of bundle override
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
Hi Daniel,
it looks good to me.
+1 to create Jira and PR.
Regards
JB
On 05/03/2019 18:19, Daniel Estermann wrote:
> Hi everybody,
>
> we have the following override.properties:
>
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT;range="[0,99999)"
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration;range="[0,99999)"
>
> which are supposed to do the following replacements:
>
> portal-backend-sdk/2.68.3 -> portal-backend-sdk/2.69.0-SNAPSHOT
>
> and
>
> portal-backend-sdk/2.68.3/jar/karaf-migration ->
> portal-backend-sdk/2.69.0-SNAPSHOT/jar/karaf-migration
>
> But the method
> org.apache.karaf.features.internal.service.FeaturesProcessorImpl.staticOverrideBundle(Bundle)
> <https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/internal/service/FeaturesProcessorImpl.java#L225>
> replaces portal-backend-sdk/2.68.3/jar/karaf-migration with
> mvn:com.seeburger.portal/portal-backend-sdk/2.69.0-SNAPSHOT, i.e. a
> classified artifact gets replaced with an artifact without classifier. This
> happens because the implementation of
> org.apache.karaf.features.LocationPattern
> <https://github.com/apache/karaf/blob/master/features/core/src/main/java/org/apache/karaf/features/LocationPattern.java#L151>
> allows matching of classified URI by a non-classified pattern. The
> LocationPatternTest indicates that this is an intentional behavior: see
> line 112
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L112>,
> 115
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L115>,
> 116
> <https://github.com/apache/karaf/blob/master/features/core/src/test/java/org/apache/karaf/features/internal/service/LocationPatternTest.java#L116>
> .
>
> I can understand why LocationPattern is implemented like that. But then my
> guess is that FeaturesProcessorImpl is incorrect. By the way an interesting
> comment there: *TOCHECK: last rule wins - no break!!!* Last rule doesn't
> work for us so looks like now the time has come to check it. I think what
> is crucial there, is that we shouldn't rely on the order of overrides, but
> rather on their quality. I.e. not the first/last match is appropriate, but
> the best one. I propose to collect candidates to match and then determine
> the best one using simply the length as criterion.
>
> My proposal is already implemented in our fork and tested on my local
> system (for now). Here you can see it:
> https://github.com/seeburger-ag/karaf/commit/940911e8a8ccdb97d5cebf976e41747f1e7716a4
>
> I would appreciate to receive any feedback on this.
>
> Regards,
> Daniel
>
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com