You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by pmouawad <gi...@git.apache.org> on 2017/09/07 20:25:36 UTC
[GitHub] jmeter issue #306: Fix to BUG_60156
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/306
Hello
Unless there is a NO GO , I'll commit this tomorrow.
Thanks
---
Re: [GitHub] jmeter issue #306: Fix to BUG_60156
Posted by Emilian Bold <em...@gmail.com>.
Looks much better now.
--emi
Pe 8 sept. 2017, la 12:04, UBIK LOAD PACK Support <su...@ubikloadpack.com> a scris:
> Hello Emilian,
> There was indeed one last issue in TCPClientImpl with backward
> compatibility, we pushed a commit
> It should be ok now, but please review (
> https://github.com/apache/jmeter/pull/306/files):
>
> - If AbstractTCPClient implement the old method they will work
> - If AbstractTCPClient implement the new method, they will of course
> work
> - If 3rd party code calls the old method, they should also work
>
>
> Regards
>
> @ubikloadpack Team
>> On Fri, Sep 8, 2017 at 6:31 AM, Emilian Bold <em...@gmail.com> wrote:
>>
>> It is backwards compatible as an SPI, meaning that 3rd party
>> implementations that use AbstractTCPClient will still work if they
>> implement only the old method.
>>
>> But it is not backwards compatible as an API because 3rd party code that
>> calls the old method will now receive nulls for those existing
>> implementations.
>>
>> So the question is if this is supposed to be an API or it's just an SPI.
>>
>> --emi
>>
>> On Fri, Sep 8, 2017 at 12:03 AM, Philippe Mouawad <
>> philippe.mouawad@gmail.com> wrote:
>>
>>> Hi Emilian
>>> AFAIU, it is backward compatible no ?
>>>
>>> in AbstractTCPClient.java
>>> <https://github.com/apache/jmeter/pull/306/files#diff-37500f
>>> a9c4a5285efe7b85e9bcd62614>,
>>> the new method calls the old one.
>>>
>>> Only the subclasses has been modified to implement the new one.
>>>
>>> Thanks for reviewing
>>>
>>> On Thu, Sep 7, 2017 at 10:58 PM, Emilian Bold <em...@gmail.com>
>>> wrote:
>>>
>>>> I don't know the codebase but why bother marking the old method as
>>>> @Deprecated if you are not making the change backwards compatible?
>>>>
>>>> I see 'return null' in two existing methods while previously those
>>> methods
>>>> returned something.
>>>>
>>>> A proper pattern would have been to call read(is, null) or read(is, new
>>>> DummySampleResult()).
>>>>
>>>>
>>>> --emi
>>>>
>>>>> On Thu, Sep 7, 2017 at 11:25 PM, pmouawad <gi...@git.apache.org> wrote:
>>>>>
>>>>> Github user pmouawad commented on the issue:
>>>>>
>>>>> https://github.com/apache/jmeter/pull/306
>>>>>
>>>>> Hello
>>>>> Unless there is a NO GO , I'll commit this tomorrow.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>> ---
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Cordialement.
>>> Philippe Mouawad.
>>>
>>
Re: [GitHub] jmeter issue #306: Fix to BUG_60156
Posted by UBIK LOAD PACK Support <su...@ubikloadpack.com>.
Hello Emilian,
There was indeed one last issue in TCPClientImpl with backward
compatibility, we pushed a commit
It should be ok now, but please review (
https://github.com/apache/jmeter/pull/306/files):
- If AbstractTCPClient implement the old method they will work
- If AbstractTCPClient implement the new method, they will of course
work
- If 3rd party code calls the old method, they should also work
Regards
@ubikloadpack Team
On Fri, Sep 8, 2017 at 6:31 AM, Emilian Bold <em...@gmail.com> wrote:
> It is backwards compatible as an SPI, meaning that 3rd party
> implementations that use AbstractTCPClient will still work if they
> implement only the old method.
>
> But it is not backwards compatible as an API because 3rd party code that
> calls the old method will now receive nulls for those existing
> implementations.
>
> So the question is if this is supposed to be an API or it's just an SPI.
>
> --emi
>
> On Fri, Sep 8, 2017 at 12:03 AM, Philippe Mouawad <
> philippe.mouawad@gmail.com> wrote:
>
> > Hi Emilian
> > AFAIU, it is backward compatible no ?
> >
> > in AbstractTCPClient.java
> > <https://github.com/apache/jmeter/pull/306/files#diff-37500f
> > a9c4a5285efe7b85e9bcd62614>,
> > the new method calls the old one.
> >
> > Only the subclasses has been modified to implement the new one.
> >
> > Thanks for reviewing
> >
> > On Thu, Sep 7, 2017 at 10:58 PM, Emilian Bold <em...@gmail.com>
> > wrote:
> >
> > > I don't know the codebase but why bother marking the old method as
> > > @Deprecated if you are not making the change backwards compatible?
> > >
> > > I see 'return null' in two existing methods while previously those
> > methods
> > > returned something.
> > >
> > > A proper pattern would have been to call read(is, null) or read(is, new
> > > DummySampleResult()).
> > >
> > >
> > > --emi
> > >
> > > On Thu, Sep 7, 2017 at 11:25 PM, pmouawad <gi...@git.apache.org> wrote:
> > >
> > > > Github user pmouawad commented on the issue:
> > > >
> > > > https://github.com/apache/jmeter/pull/306
> > > >
> > > > Hello
> > > > Unless there is a NO GO , I'll commit this tomorrow.
> > > >
> > > > Thanks
> > > >
> > > >
> > > > ---
> > > >
> > >
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
> >
>
Re: [GitHub] jmeter issue #306: Fix to BUG_60156
Posted by Emilian Bold <em...@gmail.com>.
It is backwards compatible as an SPI, meaning that 3rd party
implementations that use AbstractTCPClient will still work if they
implement only the old method.
But it is not backwards compatible as an API because 3rd party code that
calls the old method will now receive nulls for those existing
implementations.
So the question is if this is supposed to be an API or it's just an SPI.
--emi
On Fri, Sep 8, 2017 at 12:03 AM, Philippe Mouawad <
philippe.mouawad@gmail.com> wrote:
> Hi Emilian
> AFAIU, it is backward compatible no ?
>
> in AbstractTCPClient.java
> <https://github.com/apache/jmeter/pull/306/files#diff-37500f
> a9c4a5285efe7b85e9bcd62614>,
> the new method calls the old one.
>
> Only the subclasses has been modified to implement the new one.
>
> Thanks for reviewing
>
> On Thu, Sep 7, 2017 at 10:58 PM, Emilian Bold <em...@gmail.com>
> wrote:
>
> > I don't know the codebase but why bother marking the old method as
> > @Deprecated if you are not making the change backwards compatible?
> >
> > I see 'return null' in two existing methods while previously those
> methods
> > returned something.
> >
> > A proper pattern would have been to call read(is, null) or read(is, new
> > DummySampleResult()).
> >
> >
> > --emi
> >
> > On Thu, Sep 7, 2017 at 11:25 PM, pmouawad <gi...@git.apache.org> wrote:
> >
> > > Github user pmouawad commented on the issue:
> > >
> > > https://github.com/apache/jmeter/pull/306
> > >
> > > Hello
> > > Unless there is a NO GO , I'll commit this tomorrow.
> > >
> > > Thanks
> > >
> > >
> > > ---
> > >
> >
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
Re: [GitHub] jmeter issue #306: Fix to BUG_60156
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi Emilian
AFAIU, it is backward compatible no ?
in AbstractTCPClient.java
<https://github.com/apache/jmeter/pull/306/files#diff-37500fa9c4a5285efe7b85e9bcd62614>,
the new method calls the old one.
Only the subclasses has been modified to implement the new one.
Thanks for reviewing
On Thu, Sep 7, 2017 at 10:58 PM, Emilian Bold <em...@gmail.com>
wrote:
> I don't know the codebase but why bother marking the old method as
> @Deprecated if you are not making the change backwards compatible?
>
> I see 'return null' in two existing methods while previously those methods
> returned something.
>
> A proper pattern would have been to call read(is, null) or read(is, new
> DummySampleResult()).
>
>
> --emi
>
> On Thu, Sep 7, 2017 at 11:25 PM, pmouawad <gi...@git.apache.org> wrote:
>
> > Github user pmouawad commented on the issue:
> >
> > https://github.com/apache/jmeter/pull/306
> >
> > Hello
> > Unless there is a NO GO , I'll commit this tomorrow.
> >
> > Thanks
> >
> >
> > ---
> >
>
--
Cordialement.
Philippe Mouawad.
Re: [GitHub] jmeter issue #306: Fix to BUG_60156
Posted by Emilian Bold <em...@gmail.com>.
I don't know the codebase but why bother marking the old method as
@Deprecated if you are not making the change backwards compatible?
I see 'return null' in two existing methods while previously those methods
returned something.
A proper pattern would have been to call read(is, null) or read(is, new
DummySampleResult()).
--emi
On Thu, Sep 7, 2017 at 11:25 PM, pmouawad <gi...@git.apache.org> wrote:
> Github user pmouawad commented on the issue:
>
> https://github.com/apache/jmeter/pull/306
>
> Hello
> Unless there is a NO GO , I'll commit this tomorrow.
>
> Thanks
>
>
> ---
>