You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by Svetoslav Neykov <sv...@cloudsoftcorp.com> on 2015/11/16 17:15:31 UTC
Re: HttpTool backward compatibility
Geoff,
I like 4. but it might be too much work, for what it's worth - that's why 1. seems like a good alternative.
If there's no good technical reason to have HttpAsserts in utils, then +1 for (1).
Svet.
> On 16.11.2015 г., at 17:48, Geoff Macartney <ge...@cloudsoftcorp.com> wrote:
>
>
> Hi Brooklyners,
>
> As part of #994 [1], I moved HttpTool out of core into brooklyn-utils-common (leaving a deprecated HttpTools in core). This was done on the grounds that it is a tool that operates on HTTP, does not depend on brooklyn-core, and could find wider use outside core.
>
> What I didn’t reckon with is that my change broke downstream builds, because the deprecated HttpTool in core included return types from the new package (o.a.b.util.http), while its users are expecting the old packages. To fix this it’s not enough merely to restore the original HttpTool in core[2], as the HttpToolResponse from the tool ‘leaks’ into other classes, such as the response from org.apache.brooklyn.feed.http.HttpFeed#getPoller:
>
> protected Poller<HttpToolResponse> getPoller()
>
> and various methods in HttpValueFunctions, e.g.
>
> public static Function<HttpToolResponse, Boolean> containsHeader(String header)
>
> This seems worth asking the community about. What would you recommend here as a course of action?
>
> The options seem to include:
>
> 1. Moving the HttpTool and associated classes back into core. This would make the code backward compatible again but does leave the tool inside core Brooklyn, when it would be nice to have it as a util (especially when it does not in fact depend on anything in core).
> 2. Removing the deprecated classes in core, leaving the tool in brooklyn-utils-common, but changing the *package* to what the downstreams expect - org.apache.brooklyn.utils.core.http (note the “core” even though this would not be in brooklyn-core). While it might seem wrong to have a util package with “core” in its name, this is really a matter of interpretation - it could be a “core util”. On the other hand I understand there were changes made in Brooklyn recently to avoid this sort of overlapping package name, because of OSGi, so maybe this is not a possibility.
> 3. Deprecating the methods in HttpFeed and others that use the old HttpToolResponse, and providing alternatives that use the new one. I haven’t tried this but would be worried that it would result in an avalanche of deprecation.
> 4. A middle ground, which requires a bit of work:
>
> * Leave HttpTool in utils
> * extend it in core, with methods which return HttpToolResponse specialized to return the core’s package (org.apache.brooklyn.util.core.http.HttpToolResponse).
> * make HttpToolResponse a delegate to the utils version
> * Leave all code using HttpTool to use the deprecated version
>
> This last alternative would require a bit of rework in some places, at least HttpPollConfig would need to change to something that didn’t rely on the old HttpToolResponse, which in turn would mean you’d probably want to update HttpFeed to support the old HttpPollConfig (but deprecated) and the newer alternative.
>
> There may well be other approaches that we could take. I invite your suggestions as to the best approach.
>
> best regards
> Geoff
>
> [1] https://github.com/apache/incubator-brooklyn/pull/994
> [2] https://github.com/apache/incubator-brooklyn/pull/1032 <https://github.com/apache/incubator-brooklyn/pull/1032>
>
> ————————————————————
> Gnu PGP key - http://is.gd/TTTTuI
>
>
Re: HttpTool backward compatibility
Posted by Alex Heneveld <al...@cloudsoftcorp.com>.
Geoff,
I vote for leaving it as you have changed it, subject to two conditions:
* These changes should NOT make it in to a point release.
* We should put an entry in the release notes highlighting the change.
I think (4) is sometimes worth the effort within reason, but not in this
case because as you note it leaks, and the classes are not very commonly
used. The package rename problems are easily fixed when people see the
compile error coupled with the deprecation warning message; removing
deprecated imports and auto-importing the non-deprecated classes will
fix any issues.
(It would be nice if Java had an "identical to" marker for this case!)
Best
Alex
On 16/11/2015 16:15, Svetoslav Neykov wrote:
> Geoff,
> I like 4. but it might be too much work, for what it's worth - that's why 1. seems like a good alternative.
> If there's no good technical reason to have HttpAsserts in utils, then +1 for (1).
>
> Svet.
>
>
>> On 16.11.2015 г., at 17:48, Geoff Macartney <ge...@cloudsoftcorp.com> wrote:
>>
>>
>> Hi Brooklyners,
>>
>> As part of #994 [1], I moved HttpTool out of core into brooklyn-utils-common (leaving a deprecated HttpTools in core). This was done on the grounds that it is a tool that operates on HTTP, does not depend on brooklyn-core, and could find wider use outside core.
>>
>> What I didn’t reckon with is that my change broke downstream builds, because the deprecated HttpTool in core included return types from the new package (o.a.b.util.http), while its users are expecting the old packages. To fix this it’s not enough merely to restore the original HttpTool in core[2], as the HttpToolResponse from the tool ‘leaks’ into other classes, such as the response from org.apache.brooklyn.feed.http.HttpFeed#getPoller:
>>
>> protected Poller<HttpToolResponse> getPoller()
>>
>> and various methods in HttpValueFunctions, e.g.
>>
>> public static Function<HttpToolResponse, Boolean> containsHeader(String header)
>>
>> This seems worth asking the community about. What would you recommend here as a course of action?
>>
>> The options seem to include:
>>
>> 1. Moving the HttpTool and associated classes back into core. This would make the code backward compatible again but does leave the tool inside core Brooklyn, when it would be nice to have it as a util (especially when it does not in fact depend on anything in core).
>> 2. Removing the deprecated classes in core, leaving the tool in brooklyn-utils-common, but changing the *package* to what the downstreams expect - org.apache.brooklyn.utils.core.http (note the “core” even though this would not be in brooklyn-core). While it might seem wrong to have a util package with “core” in its name, this is really a matter of interpretation - it could be a “core util”. On the other hand I understand there were changes made in Brooklyn recently to avoid this sort of overlapping package name, because of OSGi, so maybe this is not a possibility.
>> 3. Deprecating the methods in HttpFeed and others that use the old HttpToolResponse, and providing alternatives that use the new one. I haven’t tried this but would be worried that it would result in an avalanche of deprecation.
>> 4. A middle ground, which requires a bit of work:
>>
>> * Leave HttpTool in utils
>> * extend it in core, with methods which return HttpToolResponse specialized to return the core’s package (org.apache.brooklyn.util.core.http.HttpToolResponse).
>> * make HttpToolResponse a delegate to the utils version
>> * Leave all code using HttpTool to use the deprecated version
>>
>> This last alternative would require a bit of rework in some places, at least HttpPollConfig would need to change to something that didn’t rely on the old HttpToolResponse, which in turn would mean you’d probably want to update HttpFeed to support the old HttpPollConfig (but deprecated) and the newer alternative.
>>
>> There may well be other approaches that we could take. I invite your suggestions as to the best approach.
>>
>> best regards
>> Geoff
>>
>> [1] https://github.com/apache/incubator-brooklyn/pull/994
>> [2] https://github.com/apache/incubator-brooklyn/pull/1032 <https://github.com/apache/incubator-brooklyn/pull/1032>
>>
>> ————————————————————
>> Gnu PGP key - http://is.gd/TTTTuI
>>
>>