You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tuscany.apache.org by an...@apache.org on 2012/03/19 09:09:05 UTC
svn commit: r1302317 -
/tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Author: antelder
Date: Mon Mar 19 08:09:04 2012
New Revision: 1302317
URL: http://svn.apache.org/viewvc?rev=1302317&view=rev
Log:
TUSCANY-4029: Clone the binding when setting it on the EndpointReference to ensure its not the same instance as that used by the Endpoint.
Modified:
tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Modified: tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
URL: http://svn.apache.org/viewvc/tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java?rev=1302317&r1=1302316&r2=1302317&view=diff
==============================================================================
--- tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java (original)
+++ tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java Mon Mar 19 08:09:04 2012
@@ -42,7 +42,6 @@ import org.apache.tuscany.sca.assembly.b
import org.apache.tuscany.sca.assembly.builder.BuilderExtensionPoint;
import org.apache.tuscany.sca.assembly.builder.CompositeBuilder;
import org.apache.tuscany.sca.assembly.builder.PolicyBuilder;
-import org.apache.tuscany.sca.assembly.xml.Messages;
import org.apache.tuscany.sca.core.ExtensionPointRegistry;
import org.apache.tuscany.sca.core.FactoryExtensionPoint;
import org.apache.tuscany.sca.core.UtilityExtensionPoint;
@@ -501,7 +500,12 @@ public class EndpointReferenceBinderImpl
} else {
endpointReference.setTargetEndpoint(matchedEndpoint);
Binding binding = matchedEndpoint.getBinding();
- endpointReference.setBinding(binding);
+ try {
+ endpointReference.setBinding((Binding) binding.clone());
+ } catch (CloneNotSupportedException e) {
+ // shouldn't happen
+ throw new RuntimeException(e);
+ }
// TUSCANY-3873 - add policy from the service
// we don't care about intents at this stage
endpointReference.getPolicySets().addAll(matchedEndpoint.getPolicySets());
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by ant elder <an...@gmail.com>.
On Wed, Mar 21, 2012 at 10:35 PM, Luciano Resende <lu...@gmail.com> wrote:
> On Wed, Mar 21, 2012 at 2:57 PM, ant elder <an...@gmail.com> wrote:
>> On Wed, Mar 21, 2012 at 9:48 PM, Raymond Feng <en...@gmail.com> wrote:
>>> Hi,
>>>
>>> The change had a huge impact on all the binding invokers that rely on the service binding to resolve the deployed URIs when the endpoint reference is resolved to a target endpoint.
>>>
>>> BTW, the use case applies to something like the following:
>>>
>>> <component name="A">
>>> <service name="S1">
>>> <tuscany:binding.rest uri="/a/b"/>
>>> </service>
>>> </component>
>>>
>>> <component name="B">
>>> <reference name="r1" target="A/S1"/>
>>> </component>
>>>
>>> The r1 reference is now have /a/b as the uri instead of the deployed URI. Ideally, the binding invoker should ask for the target endpoint's deployed URI. But it involves quite a bit changes.
>>>
>>> I'll revert the change for now until we find a consistent solution.
>>>
>>
>> Raymond, I think you know that unilaterally reverting a commit like
>> that is not the way to do things.
>>
>> ...ant
>
>
> I don't see this as "unilaterally reverting a commit". I see this as a
> commit extensively broke existing functionality, the issue was brought
> up in the mailing list for discussion of a better fix, while, in the
> mean time, the commit was reverted.
>
> Please let's concentrate on the technical facts and try to find a
> solution that works without huge impact on existing functionality.
>
Luciano, the build didn't get broken until the r1303591 commit so
phrases like "extensively broke" do nothing to constructively help
find a solution that works for everyone. There is well known and clear
ASF process for vetos and following that will help avoid these type of
exchanges. From whats been said so far about this issue it seems like
the rest binding invoker has no tests for this function and was only
working by relying on an existing bug so having that change while we
clean up trunk on the way to 2.0 is no cause for getting grumpy. There
has already been a fix for that suggested in TUSCANY-4029, if that
doesn't completely resolve the issue then respectful dev in trunk and
dev-list is the way to get to something that does.
...ant
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by Luciano Resende <lu...@gmail.com>.
On Wed, Mar 21, 2012 at 2:57 PM, ant elder <an...@gmail.com> wrote:
> On Wed, Mar 21, 2012 at 9:48 PM, Raymond Feng <en...@gmail.com> wrote:
>> Hi,
>>
>> The change had a huge impact on all the binding invokers that rely on the service binding to resolve the deployed URIs when the endpoint reference is resolved to a target endpoint.
>>
>> BTW, the use case applies to something like the following:
>>
>> <component name="A">
>> <service name="S1">
>> <tuscany:binding.rest uri="/a/b"/>
>> </service>
>> </component>
>>
>> <component name="B">
>> <reference name="r1" target="A/S1"/>
>> </component>
>>
>> The r1 reference is now have /a/b as the uri instead of the deployed URI. Ideally, the binding invoker should ask for the target endpoint's deployed URI. But it involves quite a bit changes.
>>
>> I'll revert the change for now until we find a consistent solution.
>>
>
> Raymond, I think you know that unilaterally reverting a commit like
> that is not the way to do things.
>
> ...ant
I don't see this as "unilaterally reverting a commit". I see this as a
commit extensively broke existing functionality, the issue was brought
up in the mailing list for discussion of a better fix, while, in the
mean time, the commit was reverted.
Please let's concentrate on the technical facts and try to find a
solution that works without huge impact on existing functionality.
--
Luciano Resende
http://people.apache.org/~lresende
http://twitter.com/lresende1975
http://lresende.blogspot.com/
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by ant elder <an...@gmail.com>.
On Wed, Mar 21, 2012 at 9:48 PM, Raymond Feng <en...@gmail.com> wrote:
> Hi,
>
> The change had a huge impact on all the binding invokers that rely on the service binding to resolve the deployed URIs when the endpoint reference is resolved to a target endpoint.
>
> BTW, the use case applies to something like the following:
>
> <component name="A">
> <service name="S1">
> <tuscany:binding.rest uri="/a/b"/>
> </service>
> </component>
>
> <component name="B">
> <reference name="r1" target="A/S1"/>
> </component>
>
> The r1 reference is now have /a/b as the uri instead of the deployed URI. Ideally, the binding invoker should ask for the target endpoint's deployed URI. But it involves quite a bit changes.
>
> I'll revert the change for now until we find a consistent solution.
>
Raymond, I think you know that unilaterally reverting a commit like
that is not the way to do things.
...ant
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by Raymond Feng <en...@gmail.com>.
Hi,
The change had a huge impact on all the binding invokers that rely on the service binding to resolve the deployed URIs when the endpoint reference is resolved to a target endpoint.
BTW, the use case applies to something like the following:
<component name="A">
<service name="S1">
<tuscany:binding.rest uri="/a/b"/>
</service>
</component>
<component name="B">
<reference name="r1" target="A/S1"/>
</component>
The r1 reference is now have /a/b as the uri instead of the deployed URI. Ideally, the binding invoker should ask for the target endpoint's deployed URI. But it involves quite a bit changes.
I'll revert the change for now until we find a consistent solution.
Thanks,
Raymond
On Mar 21, 2012, at 2:01 PM, Raymond Feng wrote:
> Hi,
>
> This change breaks the case where we use @target in the <reference> element to wire components using non-SCA bindings. For example, the RESTBindingInvoker uses the binding.uri to find out the target address. Since now it's a clone, the value won't be updated when the service binding set the deployed URI.
>
> A better fix is to retrieve the deployed URI from the target endpoint.
>
> Thanks,
> Raymond
>
> On Mar 19, 2012, at 1:09 AM, antelder@apache.org wrote:
>
>> Author: antelder
>> Date: Mon Mar 19 08:09:04 2012
>> New Revision: 1302317
>>
>> URL: http://svn.apache.org/viewvc?rev=1302317&view=rev
>> Log:
>> TUSCANY-4029: Clone the binding when setting it on the EndpointReference to ensure its not the same instance as that used by the Endpoint.
>>
>> Modified:
>> tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
>>
>> Modified: tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
>> URL: http://svn.apache.org/viewvc/tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java?rev=1302317&r1=1302316&r2=1302317&view=diff
>> ==============================================================================
>> --- tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java (original)
>> +++ tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java Mon Mar 19 08:09:04 2012
>> @@ -42,7 +42,6 @@ import org.apache.tuscany.sca.assembly.b
>> import org.apache.tuscany.sca.assembly.builder.BuilderExtensionPoint;
>> import org.apache.tuscany.sca.assembly.builder.CompositeBuilder;
>> import org.apache.tuscany.sca.assembly.builder.PolicyBuilder;
>> -import org.apache.tuscany.sca.assembly.xml.Messages;
>> import org.apache.tuscany.sca.core.ExtensionPointRegistry;
>> import org.apache.tuscany.sca.core.FactoryExtensionPoint;
>> import org.apache.tuscany.sca.core.UtilityExtensionPoint;
>> @@ -501,7 +500,12 @@ public class EndpointReferenceBinderImpl
>> } else {
>> endpointReference.setTargetEndpoint(matchedEndpoint);
>> Binding binding = matchedEndpoint.getBinding();
>> - endpointReference.setBinding(binding);
>> + try {
>> + endpointReference.setBinding((Binding) binding.clone());
>> + } catch (CloneNotSupportedException e) {
>> + // shouldn't happen
>> + throw new RuntimeException(e);
>> + }
>> // TUSCANY-3873 - add policy from the service
>> // we don't care about intents at this stage
>> endpointReference.getPolicySets().addAll(matchedEndpoint.getPolicySets());
>>
>>
>
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by ant elder <an...@gmail.com>.
On Wed, Mar 21, 2012 at 9:01 PM, Raymond Feng <en...@gmail.com> wrote:
> Hi,
>
> This change breaks the case where we use @target in the <reference> element to wire components using non-SCA bindings. For example, the RESTBindingInvoker uses the binding.uri to find out the target address. Since now it's a clone, the value won't be updated when the service binding set the deployed URI.
>
> A better fix is to retrieve the deployed URI from the target endpoint.
>
I agree its irksome that simple local @target references can't rely on
getting the service url set after its wired up but really that seems
like it was only working previously by relying on the bug that the
objects were shared.
Getting the URI from the target endpoint isn't going to solve all the
other problems that can happen when the Binding object is shared, the
problem is that what information within a Binding can or can't be
shared between the Endpoint and EndpointReference is difficult to
determine and varies across binding impls - policy, databindings,
really any of the binding fields - getting set by one side can easily
overwrite what the other end had previously set.
I can't yet see a better way to fix this than cloning the binding
here. What about trying to fix the deployed service URI getting passed
on to the reference by doing something with the EndpointRegistry so
that setting the deployed URI on the service Endpoint in there gets
its replicated to the reference side?
...ant
Re: svn commit: r1302317 - /tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
Posted by Raymond Feng <en...@gmail.com>.
Hi,
This change breaks the case where we use @target in the <reference> element to wire components using non-SCA bindings. For example, the RESTBindingInvoker uses the binding.uri to find out the target address. Since now it's a clone, the value won't be updated when the service binding set the deployed URI.
A better fix is to retrieve the deployed URI from the target endpoint.
Thanks,
Raymond
On Mar 19, 2012, at 1:09 AM, antelder@apache.org wrote:
> Author: antelder
> Date: Mon Mar 19 08:09:04 2012
> New Revision: 1302317
>
> URL: http://svn.apache.org/viewvc?rev=1302317&view=rev
> Log:
> TUSCANY-4029: Clone the binding when setting it on the EndpointReference to ensure its not the same instance as that used by the Endpoint.
>
> Modified:
> tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
>
> Modified: tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java
> URL: http://svn.apache.org/viewvc/tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java?rev=1302317&r1=1302316&r2=1302317&view=diff
> ==============================================================================
> --- tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java (original)
> +++ tuscany/sca-java-2.x/trunk/modules/core/src/main/java/org/apache/tuscany/sca/core/runtime/impl/EndpointReferenceBinderImpl.java Mon Mar 19 08:09:04 2012
> @@ -42,7 +42,6 @@ import org.apache.tuscany.sca.assembly.b
> import org.apache.tuscany.sca.assembly.builder.BuilderExtensionPoint;
> import org.apache.tuscany.sca.assembly.builder.CompositeBuilder;
> import org.apache.tuscany.sca.assembly.builder.PolicyBuilder;
> -import org.apache.tuscany.sca.assembly.xml.Messages;
> import org.apache.tuscany.sca.core.ExtensionPointRegistry;
> import org.apache.tuscany.sca.core.FactoryExtensionPoint;
> import org.apache.tuscany.sca.core.UtilityExtensionPoint;
> @@ -501,7 +500,12 @@ public class EndpointReferenceBinderImpl
> } else {
> endpointReference.setTargetEndpoint(matchedEndpoint);
> Binding binding = matchedEndpoint.getBinding();
> - endpointReference.setBinding(binding);
> + try {
> + endpointReference.setBinding((Binding) binding.clone());
> + } catch (CloneNotSupportedException e) {
> + // shouldn't happen
> + throw new RuntimeException(e);
> + }
> // TUSCANY-3873 - add policy from the service
> // we don't care about intents at this stage
> endpointReference.getPolicySets().addAll(matchedEndpoint.getPolicySets());
>
>