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 <[email protected]>
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
