You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@serf.apache.org by Lieven Govaerts <lg...@mobsol.be> on 2015/09/06 23:25:09 UTC
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Hey Ivan,
On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 6 April 2015 at 20:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 6 April 2015 at 19:02, Lieven Govaerts <lg...@mobsol.be> wrote:
>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>>>> -----Original Message-----
>>>>> From: serf-dev@googlegroups.com [mailto:serf-dev@googlegroups.com] On
>>>>> Behalf Of serf@googlecode.com
>>>>> Sent: maandag 6 april 2015 11:25
>>>>> To: serf-dev@googlegroups.com
>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
>>>>> remove the get_remaining function from t...
>>>>>
>>>>> Revision: 2489
>>>>> Author: lieven.govaerts
>>>>> Date: Mon Apr 6 09:24:18 2015 UTC
>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining function
>>>>> from the
>>>>> bucket API.
>>>>>
>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I kept the
>>>>> read_bucket_v2 function, which is needed for set_config.
>>>>
>>>> If we still keep the read_bucket_v2 feature, what is the reason for just removing get_remaining?
>>>>
>>>
>>> I'm fixing all TODO's as discussed in the summer last year.
>>> Get_remaining isn't finished yet and not used. So instead of waiting
>>> until someone uses it, I'm removing it now so we can get 1.4 branched.
>>>
>>> We can still revert this revision after 1.4.x is branched though. In
>>> fact, we can release 1.5.x with just this if an application wants to
>>> make use of the (finalized) get_remaining feature.
>>>
>> What problem do we have with get_remaining() feature except
>> read_bucket_v2() linkage problems? get_remaining() feature is not used
>> in Subversion for only one reason: serf-trunk has version 2.0.0 so
>> it's not possible to add version detection code.
Huh? We always used a check on the next version of serf to check for
trunk code, why is that not possible anymore?
It's 2 years after you added the get_remaining API, and it's still not
used in Subversion.
>>
>>
> Hi Lieven,
>
> I still don't understand your arguments on reverting get_remaining()
> feature. Why you consider get_remaining() as isn't finished?
I consider it as not finished because many bucket types don't have a
get_remaining implementation, not even a default function, just NULL.
If you use such a bucket in an aggregate bucket, you'll get a quite nasty crash.
> The read_bucket_v2() linking problem also apply to your serf_config_t
> feature, but we didn't reverted it from trunk.
>
> Also adding this feature latter will require read_bucket_v3() which
> increase the mess.
>
> --
> Ivan Zhakov
My guess was then, and still is, that this feature can wait a release.
I'm not against the feature, I'm against adding features that will not
be used.
Lieven
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 22:59, Lieven Govaerts <lg...@mobsol.be> wrote:
> On Mon, Sep 7, 2015 at 9:35 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 7 September 2015 at 00:25, Lieven Govaerts <lg...@mobsol.be> wrote:
>>> Hey Ivan,
>>>
>>> On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>> On 6 April 2015 at 20:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>> On 6 April 2015 at 19:02, Lieven Govaerts <lg...@mobsol.be> wrote:
>>>>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: serf-dev@googlegroups.com [mailto:serf-dev@googlegroups.com] On
>>>>>>>> Behalf Of serf@googlecode.com
>>>>>>>> Sent: maandag 6 april 2015 11:25
>>>>>>>> To: serf-dev@googlegroups.com
>>>>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
>>>>>>>> remove the get_remaining function from t...
>>>>>>>>
>>>>>>>> Revision: 2489
>>>>>>>> Author: lieven.govaerts
>>>>>>>> Date: Mon Apr 6 09:24:18 2015 UTC
>>>>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining function
>>>>>>>> from the
>>>>>>>> bucket API.
>>>>>>>>
>>>>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I kept the
>>>>>>>> read_bucket_v2 function, which is needed for set_config.
>>>>>>>
>>>>>>> If we still keep the read_bucket_v2 feature, what is the reason for just removing get_remaining?
>>>>>>>
>>>>>>
>>>>>> I'm fixing all TODO's as discussed in the summer last year.
>>>>>> Get_remaining isn't finished yet and not used. So instead of waiting
>>>>>> until someone uses it, I'm removing it now so we can get 1.4 branched.
>>>>>>
>>>>>> We can still revert this revision after 1.4.x is branched though. In
>>>>>> fact, we can release 1.5.x with just this if an application wants to
>>>>>> make use of the (finalized) get_remaining feature.
>>>>>>
>>>>> What problem do we have with get_remaining() feature except
>>>>> read_bucket_v2() linkage problems? get_remaining() feature is not used
>>>>> in Subversion for only one reason: serf-trunk has version 2.0.0 so
>>>>> it's not possible to add version detection code.
>>>
>>> Huh? We always used a check on the next version of serf to check for
>>> trunk code, why is that not possible anymore?
>>> It's 2 years after you added the get_remaining API, and it's still not
>>> used in Subversion.
>>>
>> Current version in serf trunk is 2.0.0. I posted patch to use
>> get_remaining() API on Subversion list two years ago [1]. The only
>> reason it wasn't committed because serf-trunk has version 2.0.0
>> instead of 1.4.0, so I cannot complete my patch with correct API
>> check.
>
> Okay.
>
>>>>
>>>> I still don't understand your arguments on reverting get_remaining()
>>>> feature. Why you consider get_remaining() as isn't finished?
>>>
>>> I consider it as not finished because many bucket types don't have a
>>> get_remaining implementation, not even a default function, just NULL.
>>> If you use such a bucket in an aggregate bucket, you'll get a quite nasty crash.
>>>
>> Some buckets doesn't have get_remaining() implementation by design:
>> not every bucket type know data length. And
>> serf_bucket_get_remaining() returns SERF_LENGTH_UNKNOWN in this case.
>
> No it doesn't.
>
> +#define serf_bucket_get_remaining(b) \
> + ((b)->type->read_bucket == serf_buckets_are_v2 ? \
> + (b)->type->get_remaining(b) : \
> + SERF_LENGTH_UNKNOWN)
>
> The serf_bucket_get_remaining macro will return SERF_LENGTH_UNKNOWN
> for non-v2 buckets. Your macro assumes that all v2 buckets implement
> get_remaining!
> To be fair to you, that was true at the time you wrote this code. But
> now that the get_config API is there too, if we apply this
> get_remaining patch again we'll have v2 buckets with NULL as
> get_remaining pointer.
So, the get_remaining() code was correct, but broken with config store
implementation which changed all buckets to be v2?
> So I'm against using NULL as value for function pointers in the bucket
> definitions, to avoid problems like this.
>
> A simple fix would be to create a serf_default_get_remaining() which
> returns SERF_LENGTH_UNKNOWN, and use that in all the serf bucket
> types.
>
It seems to be good improvement anyway.
>> Also I think it's better to report bug or fix it instead of reverting
>> feature from trunk especially without prior discussion.
>
> This was discussed, in the thread initially planning the 1.4.0 release
> more than a year ago.
>
> I just executed on my proposed plan to prepare for 1.4.0. This feature
> wasn't vetoed or anything, my goal was to remove it from trunk
> temporarily to get 1.4.0 out with features that were finished and
> wanted. Obviously the assumption was that we would release 1.5.0 a
> couple of months later with the get_remaining() API back in. We've
> just been a bit slower than that. ;)
>
Ack.
>>>> The read_bucket_v2() linking problem also apply to your serf_config_t
>>>> feature, but we didn't reverted it from trunk.
>>>>
>>>> Also adding this feature latter will require read_bucket_v3() which
>>>> increase the mess.
>
> Yeah, that's a bit annoying. If you prefer to reinstate this code,
> with the default implementation of get_remaining, then I'm fine with
> that.
I'm going to reinstate get_remaining() code if you don't have
objections: I think it useful functionality anyway.
We still have to find way to fix read_bucket_v2() linkage problem, but
it's unrelated problem. My current analysis is that making serf 2.0 is
the only option to have v2 buckets reliable.
>>> My guess was then, and still is, that this feature can wait a release.
>>> I'm not against the feature, I'm against adding features that will not
>>> be used.
>>>
>> Of course we should not block release because of this feature, but you
>> reverted completed functionality from serf trunk with justification
>> that it's not used by one of serf user (Subversion). But how
>> Subversion should work if patch with use of serf_bucket_get_remaining
>> was committed to Subversion and released in 1.9.0 and then Serf
>> developers decide that serf_bucket_get_remaining() will not be
>> released in serf 1.4.0? Subversion cannot depends on serf
>> functionality that is not released, because serf developers may change
>> everything before release.
>
> ..
>> Subversion cannot rely on unreleased serf
>> features.
>
> Agreed.
>
--
Ivan Zhakov
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Lieven Govaerts <lg...@mobsol.be>.
On Mon, Sep 7, 2015 at 9:35 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 September 2015 at 00:25, Lieven Govaerts <lg...@mobsol.be> wrote:
>> Hey Ivan,
>>
>> On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On 6 April 2015 at 20:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>> On 6 April 2015 at 19:02, Lieven Govaerts <lg...@mobsol.be> wrote:
>>>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: serf-dev@googlegroups.com [mailto:serf-dev@googlegroups.com] On
>>>>>>> Behalf Of serf@googlecode.com
>>>>>>> Sent: maandag 6 april 2015 11:25
>>>>>>> To: serf-dev@googlegroups.com
>>>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
>>>>>>> remove the get_remaining function from t...
>>>>>>>
>>>>>>> Revision: 2489
>>>>>>> Author: lieven.govaerts
>>>>>>> Date: Mon Apr 6 09:24:18 2015 UTC
>>>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining function
>>>>>>> from the
>>>>>>> bucket API.
>>>>>>>
>>>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I kept the
>>>>>>> read_bucket_v2 function, which is needed for set_config.
>>>>>>
>>>>>> If we still keep the read_bucket_v2 feature, what is the reason for just removing get_remaining?
>>>>>>
>>>>>
>>>>> I'm fixing all TODO's as discussed in the summer last year.
>>>>> Get_remaining isn't finished yet and not used. So instead of waiting
>>>>> until someone uses it, I'm removing it now so we can get 1.4 branched.
>>>>>
>>>>> We can still revert this revision after 1.4.x is branched though. In
>>>>> fact, we can release 1.5.x with just this if an application wants to
>>>>> make use of the (finalized) get_remaining feature.
>>>>>
>>>> What problem do we have with get_remaining() feature except
>>>> read_bucket_v2() linkage problems? get_remaining() feature is not used
>>>> in Subversion for only one reason: serf-trunk has version 2.0.0 so
>>>> it's not possible to add version detection code.
>>
>> Huh? We always used a check on the next version of serf to check for
>> trunk code, why is that not possible anymore?
>> It's 2 years after you added the get_remaining API, and it's still not
>> used in Subversion.
>>
> Current version in serf trunk is 2.0.0. I posted patch to use
> get_remaining() API on Subversion list two years ago [1]. The only
> reason it wasn't committed because serf-trunk has version 2.0.0
> instead of 1.4.0, so I cannot complete my patch with correct API
> check.
Okay.
>
>>>
>>> I still don't understand your arguments on reverting get_remaining()
>>> feature. Why you consider get_remaining() as isn't finished?
>>
>> I consider it as not finished because many bucket types don't have a
>> get_remaining implementation, not even a default function, just NULL.
>> If you use such a bucket in an aggregate bucket, you'll get a quite nasty crash.
>>
> Some buckets doesn't have get_remaining() implementation by design:
> not every bucket type know data length. And
> serf_bucket_get_remaining() returns SERF_LENGTH_UNKNOWN in this case.
No it doesn't.
+#define serf_bucket_get_remaining(b) \
+ ((b)->type->read_bucket == serf_buckets_are_v2 ? \
+ (b)->type->get_remaining(b) : \
+ SERF_LENGTH_UNKNOWN)
The serf_bucket_get_remaining macro will return SERF_LENGTH_UNKNOWN
for non-v2 buckets. Your macro assumes that all v2 buckets implement
get_remaining!
To be fair to you, that was true at the time you wrote this code. But
now that the get_config API is there too, if we apply this
get_remaining patch again we'll have v2 buckets with NULL as
get_remaining pointer.
So I'm against using NULL as value for function pointers in the bucket
definitions, to avoid problems like this.
A simple fix would be to create a serf_default_get_remaining() which
returns SERF_LENGTH_UNKNOWN, and use that in all the serf bucket
types.
> Also I think it's better to report bug or fix it instead of reverting
> feature from trunk especially without prior discussion.
This was discussed, in the thread initially planning the 1.4.0 release
more than a year ago.
I just executed on my proposed plan to prepare for 1.4.0. This feature
wasn't vetoed or anything, my goal was to remove it from trunk
temporarily to get 1.4.0 out with features that were finished and
wanted. Obviously the assumption was that we would release 1.5.0 a
couple of months later with the get_remaining() API back in. We've
just been a bit slower than that. ;)
>
>>> The read_bucket_v2() linking problem also apply to your serf_config_t
>>> feature, but we didn't reverted it from trunk.
>>>
>>> Also adding this feature latter will require read_bucket_v3() which
>>> increase the mess.
Yeah, that's a bit annoying. If you prefer to reinstate this code,
with the default implementation of get_remaining, then I'm fine with
that.
>>>
>> My guess was then, and still is, that this feature can wait a release.
>> I'm not against the feature, I'm against adding features that will not
>> be used.
>>
> Of course we should not block release because of this feature, but you
> reverted completed functionality from serf trunk with justification
> that it's not used by one of serf user (Subversion). But how
> Subversion should work if patch with use of serf_bucket_get_remaining
> was committed to Subversion and released in 1.9.0 and then Serf
> developers decide that serf_bucket_get_remaining() will not be
> released in serf 1.4.0? Subversion cannot depends on serf
> functionality that is not released, because serf developers may change
> everything before release.
..
> Subversion cannot rely on unreleased serf
> features.
Agreed.
regards,
Lieven
>
> [1] http://svn.haxx.se/dev/archive-2013-07/0133.shtml
>
> --
> Ivan Zhakov
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Lieven Govaerts <lg...@mobsol.be>.
On Mon, Sep 7, 2015 at 12:33 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein@gmail.com]
>> Sent: maandag 7 september 2015 11:29
>> To: Ivan Zhakov <iv...@visualsvn.com>
>> Cc: dev@serf.apache.org
>> Subject: Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
>> remove the get_remaining function from t...
>>
>> How about: stop the debate? Remove it for 1.4.x (done), and bring it back
>> for 1.5.x? ... The stuff I was doing on trunk for 2.x, I will move to a
>> branch. Trunk will be 1.x.
>>
>> There is no reason to keep get_remaining in 1.4. It sounds like we have
>> further work to keep compatibility regardless. Some Windows linkage thing,
>> that I don't entirely understand.
>>
>> But let's move forward, rather than complain. There is a 1.4.x release to
>> get out.
>
> There are still two test errors on at least two major platforms:
> Windows reports
> 1) test_ssl_missing_client_certificate: /srv/subversion/buildbot/slave/serf-x64-macosx/build/test/test_ssl.c:1815: expected <120172> but was <120199>
This test works most of the time, but will fail on slower systems, or
with logging enabled. I think it's caused by an issue in the test
framework but I doubt this test is really useful.
What it tests, is that serf handles an SSL Alert with the correct
error code. The SSL Alert is normally sent by the test server when it
expects a client certificate, but the test explicitly doesn't send
one.
The test fails when the server didn't have the chance to send the
alert bytes, but for that it gets only one chance, as the server loop
isn't repeated as soon as the test framework reports an error.
We can disable this test for now.
> 2) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx/build/test/test_util.c:541: expected <0> but was <120199>
> (See https://ci.apache.org/builders/serf-x64-macosx/builds/6/steps/Test/logs/stdio)
>
> And Mac reports with openssl 0.9.8
> 1) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx-default-openssl/build/test/test_util.c:541: expected <0> but was <120199>
> (See https://ci.apache.org/builders/serf-x64-macosx-default-openssl/builds/1/steps/Test/logs/stdio)
I managed to fix this test by recreating all certificates using the
create_certs.py script. Not sure what changes though, the certificate
used by this test is very simple (just like the others). Worth looking
into a bit deeper.
>
> And with a more recent openssl:
> 1) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx-apr1_5/build/test/test_util.c:541: expected <0> but was <120199>
> 2) test_ssl_renegotiate: /srv/subversion/buildbot/slave/serf-x64-macosx-apr1_5/build/test/test_ssl.c:1771: expected <0> but was <120199>
> (See https://ci.apache.org/builders/serf-x64-macosx-apr1.5/builds/5/steps/Test/logs/stdio)
>
> I think I've had these errors on Windows for at least 3 months, probably longer.
> (120199 is a generic serf test failure. The cases I looked at return this from the test http server)
>
>
> I have a hard time understanding these tests and I don't know what it would take to resolve these.
>
Enabling logging in serf and the test framework would be a good start I guess.
>
> Bert
>
Lieven
RE: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0, remove the get_remaining function from t...
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: maandag 7 september 2015 11:29
> To: Ivan Zhakov <iv...@visualsvn.com>
> Cc: dev@serf.apache.org
> Subject: Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
> remove the get_remaining function from t...
>
> How about: stop the debate? Remove it for 1.4.x (done), and bring it back
> for 1.5.x? ... The stuff I was doing on trunk for 2.x, I will move to a
> branch. Trunk will be 1.x.
>
> There is no reason to keep get_remaining in 1.4. It sounds like we have
> further work to keep compatibility regardless. Some Windows linkage thing,
> that I don't entirely understand.
>
> But let's move forward, rather than complain. There is a 1.4.x release to
> get out.
There are still two test errors on at least two major platforms:
Windows reports
1) test_ssl_missing_client_certificate: /srv/subversion/buildbot/slave/serf-x64-macosx/build/test/test_ssl.c:1815: expected <120172> but was <120199>
2) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx/build/test/test_util.c:541: expected <0> but was <120199>
(See https://ci.apache.org/builders/serf-x64-macosx/builds/6/steps/Test/logs/stdio)
And Mac reports with openssl 0.9.8
1) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx-default-openssl/build/test/test_util.c:541: expected <0> but was <120199>
(See https://ci.apache.org/builders/serf-x64-macosx-default-openssl/builds/1/steps/Test/logs/stdio)
And with a more recent openssl:
1) test_ssl_server_cert_with_san_and_empty_cb: /srv/subversion/buildbot/slave/serf-x64-macosx-apr1_5/build/test/test_util.c:541: expected <0> but was <120199>
2) test_ssl_renegotiate: /srv/subversion/buildbot/slave/serf-x64-macosx-apr1_5/build/test/test_ssl.c:1771: expected <0> but was <120199>
(See https://ci.apache.org/builders/serf-x64-macosx-apr1.5/builds/5/steps/Test/logs/stdio)
I think I've had these errors on Windows for at least 3 months, probably longer.
(120199 is a generic serf test failure. The cases I looked at return this from the test http server)
I have a hard time understanding these tests and I don't know what it would take to resolve these.
Bert
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 12:28, Greg Stein <gs...@gmail.com> wrote:
> How about: stop the debate? Remove it for 1.4.x (done), and bring it back
> for 1.5.x? ... The stuff I was doing on trunk for 2.x, I will move to a
> branch. Trunk will be 1.x.
>
That would be hard to do, because we had to introduce read_bucket_v3()
in this case. I think the mess doesn't worth it.
> There is no reason to keep get_remaining in 1.4. It sounds like we have
> further work to keep compatibility regardless. Some Windows linkage thing,
> that I don't entirely understand.
>
We have problem with read_bucket_v2() approach [1] to detect buckets
with newer compatibility: we don't have guarantee that function
pointers are the same for library itself and library users on unix. As
far I understand this how LD works.
We use this functionality for get_remaining() *and* bucket config store.
[1] https://groups.google.com/d/msg/serf-dev/DtemuQPk2BE/Q86oGn7LlXkJ
--
Ivan Zhakov
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Greg Stein <gs...@gmail.com>.
How about: stop the debate? Remove it for 1.4.x (done), and bring it back
for 1.5.x? ... The stuff I was doing on trunk for 2.x, I will move to a
branch. Trunk will be 1.x.
There is no reason to keep get_remaining in 1.4. It sounds like we have
further work to keep compatibility regardless. Some Windows linkage thing,
that I don't entirely understand.
But let's move forward, rather than complain. There is a 1.4.x release to
get out.
Thx,
-g
On Sep 7, 2015 3:36 PM, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
> On 7 September 2015 at 00:25, Lieven Govaerts <lg...@mobsol.be> wrote:
> > Hey Ivan,
> >
> > On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> On 6 April 2015 at 20:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>> On 6 April 2015 at 19:02, Lieven Govaerts <lg...@mobsol.be> wrote:
> >>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: serf-dev@googlegroups.com [mailto:serf-dev@googlegroups.com]
> On
> >>>>>> Behalf Of serf@googlecode.com
> >>>>>> Sent: maandag 6 april 2015 11:25
> >>>>>> To: serf-dev@googlegroups.com
> >>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf
> 1.4.0,
> >>>>>> remove the get_remaining function from t...
> >>>>>>
> >>>>>> Revision: 2489
> >>>>>> Author: lieven.govaerts
> >>>>>> Date: Mon Apr 6 09:24:18 2015 UTC
> >>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining
> function
> >>>>>> from the
> >>>>>> bucket API.
> >>>>>>
> >>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I
> kept the
> >>>>>> read_bucket_v2 function, which is needed for set_config.
> >>>>>
> >>>>> If we still keep the read_bucket_v2 feature, what is the reason for
> just removing get_remaining?
> >>>>>
> >>>>
> >>>> I'm fixing all TODO's as discussed in the summer last year.
> >>>> Get_remaining isn't finished yet and not used. So instead of waiting
> >>>> until someone uses it, I'm removing it now so we can get 1.4 branched.
> >>>>
> >>>> We can still revert this revision after 1.4.x is branched though. In
> >>>> fact, we can release 1.5.x with just this if an application wants to
> >>>> make use of the (finalized) get_remaining feature.
> >>>>
> >>> What problem do we have with get_remaining() feature except
> >>> read_bucket_v2() linkage problems? get_remaining() feature is not used
> >>> in Subversion for only one reason: serf-trunk has version 2.0.0 so
> >>> it's not possible to add version detection code.
> >
> > Huh? We always used a check on the next version of serf to check for
> > trunk code, why is that not possible anymore?
> > It's 2 years after you added the get_remaining API, and it's still not
> > used in Subversion.
> >
> Current version in serf trunk is 2.0.0. I posted patch to use
> get_remaining() API on Subversion list two years ago [1]. The only
> reason it wasn't committed because serf-trunk has version 2.0.0
> instead of 1.4.0, so I cannot complete my patch with correct API
> check.
>
> >>
> >> I still don't understand your arguments on reverting get_remaining()
> >> feature. Why you consider get_remaining() as isn't finished?
> >
> > I consider it as not finished because many bucket types don't have a
> > get_remaining implementation, not even a default function, just NULL.
> > If you use such a bucket in an aggregate bucket, you'll get a quite
> nasty crash.
> >
> Some buckets doesn't have get_remaining() implementation by design:
> not every bucket type know data length. And
> serf_bucket_get_remaining() returns SERF_LENGTH_UNKNOWN in this case.
> Also I think it's better to report bug or fix it instead of reverting
> feature from trunk especially without prior discussion.
>
> >> The read_bucket_v2() linking problem also apply to your serf_config_t
> >> feature, but we didn't reverted it from trunk.
> >>
> >> Also adding this feature latter will require read_bucket_v3() which
> >> increase the mess.
> >>
> > My guess was then, and still is, that this feature can wait a release.
> > I'm not against the feature, I'm against adding features that will not
> > be used.
> >
> Of course we should not block release because of this feature, but you
> reverted completed functionality from serf trunk with justification
> that it's not used by one of serf user (Subversion). But how
> Subversion should work if patch with use of serf_bucket_get_remaining
> was committed to Subversion and released in 1.9.0 and then Serf
> developers decide that serf_bucket_get_remaining() will not be
> released in serf 1.4.0? Subversion cannot depends on serf
> functionality that is not released, because serf developers may change
> everything before release. Subversion cannot rely on unreleased serf
> features.
>
> [1] http://svn.haxx.se/dev/archive-2013-07/0133.shtml
>
> --
> Ivan Zhakov
>
Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
remove the get_remaining function from t...
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 00:25, Lieven Govaerts <lg...@mobsol.be> wrote:
> Hey Ivan,
>
> On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 6 April 2015 at 20:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On 6 April 2015 at 19:02, Lieven Govaerts <lg...@mobsol.be> wrote:
>>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>>>>> -----Original Message-----
>>>>>> From: serf-dev@googlegroups.com [mailto:serf-dev@googlegroups.com] On
>>>>>> Behalf Of serf@googlecode.com
>>>>>> Sent: maandag 6 april 2015 11:25
>>>>>> To: serf-dev@googlegroups.com
>>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0,
>>>>>> remove the get_remaining function from t...
>>>>>>
>>>>>> Revision: 2489
>>>>>> Author: lieven.govaerts
>>>>>> Date: Mon Apr 6 09:24:18 2015 UTC
>>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining function
>>>>>> from the
>>>>>> bucket API.
>>>>>>
>>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I kept the
>>>>>> read_bucket_v2 function, which is needed for set_config.
>>>>>
>>>>> If we still keep the read_bucket_v2 feature, what is the reason for just removing get_remaining?
>>>>>
>>>>
>>>> I'm fixing all TODO's as discussed in the summer last year.
>>>> Get_remaining isn't finished yet and not used. So instead of waiting
>>>> until someone uses it, I'm removing it now so we can get 1.4 branched.
>>>>
>>>> We can still revert this revision after 1.4.x is branched though. In
>>>> fact, we can release 1.5.x with just this if an application wants to
>>>> make use of the (finalized) get_remaining feature.
>>>>
>>> What problem do we have with get_remaining() feature except
>>> read_bucket_v2() linkage problems? get_remaining() feature is not used
>>> in Subversion for only one reason: serf-trunk has version 2.0.0 so
>>> it's not possible to add version detection code.
>
> Huh? We always used a check on the next version of serf to check for
> trunk code, why is that not possible anymore?
> It's 2 years after you added the get_remaining API, and it's still not
> used in Subversion.
>
Current version in serf trunk is 2.0.0. I posted patch to use
get_remaining() API on Subversion list two years ago [1]. The only
reason it wasn't committed because serf-trunk has version 2.0.0
instead of 1.4.0, so I cannot complete my patch with correct API
check.
>>
>> I still don't understand your arguments on reverting get_remaining()
>> feature. Why you consider get_remaining() as isn't finished?
>
> I consider it as not finished because many bucket types don't have a
> get_remaining implementation, not even a default function, just NULL.
> If you use such a bucket in an aggregate bucket, you'll get a quite nasty crash.
>
Some buckets doesn't have get_remaining() implementation by design:
not every bucket type know data length. And
serf_bucket_get_remaining() returns SERF_LENGTH_UNKNOWN in this case.
Also I think it's better to report bug or fix it instead of reverting
feature from trunk especially without prior discussion.
>> The read_bucket_v2() linking problem also apply to your serf_config_t
>> feature, but we didn't reverted it from trunk.
>>
>> Also adding this feature latter will require read_bucket_v3() which
>> increase the mess.
>>
> My guess was then, and still is, that this feature can wait a release.
> I'm not against the feature, I'm against adding features that will not
> be used.
>
Of course we should not block release because of this feature, but you
reverted completed functionality from serf trunk with justification
that it's not used by one of serf user (Subversion). But how
Subversion should work if patch with use of serf_bucket_get_remaining
was committed to Subversion and released in 1.9.0 and then Serf
developers decide that serf_bucket_get_remaining() will not be
released in serf 1.4.0? Subversion cannot depends on serf
functionality that is not released, because serf developers may change
everything before release. Subversion cannot rely on unreleased serf
features.
[1] http://svn.haxx.se/dev/archive-2013-07/0133.shtml
--
Ivan Zhakov