You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by ubikloadpack <gi...@git.apache.org> on 2017/09/02 20:17:26 UTC
[GitHub] jmeter pull request #306: Fix to BUG_60156
GitHub user ubikloadpack opened a pull request:
https://github.com/apache/jmeter/pull/306
Fix to BUG_60156
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ubikloadpack/jmeter BUG_60156
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jmeter/pull/306.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #306
----
commit a25764304ce0589acb357bbac2dc1b5085d598fb
Author: pmouawad <p....@ubik-ingenierie.com>
Date: 2017-09-02T20:15:47Z
Fix to BUG_60156
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter pull request #306: Fix to BUG_60156
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jmeter/pull/306
---
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
>
>
> ---
>
[GitHub] jmeter issue #306: Fix to BUG_60156
Posted by pmouawad <gi...@git.apache.org>.
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
---
[GitHub] jmeter issue #306: Fix to BUG_60156
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/306
This PR was merged through svn commit http://svn.apache.org/viewvc?rev=1807719&view=rev
---
[GitHub] jmeter issue #306: Fix to BUG_60156
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/306
Thanks @emilianbold for review.
---