serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Zhakov <i...@visualsvn.com>
Subject Re: [serf-dev] [serf] r2489 committed - In preparation of serf 1.4.0, remove the get_remaining function from t...
Date Tue, 08 Sep 2015 12:44:49 GMT
On 7 September 2015 at 22:59, Lieven Govaerts <lgo@mobsol.be> wrote:
> On Mon, Sep 7, 2015 at 9:35 AM, Ivan Zhakov <ivan@visualsvn.com> wrote:
>> On 7 September 2015 at 00:25, Lieven Govaerts <lgo@mobsol.be> wrote:
>>> Hey Ivan,
>>>
>>> On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <ivan@visualsvn.com> wrote:
>>>> On 6 April 2015 at 20:49, Ivan Zhakov <ivan@visualsvn.com> wrote:
>>>>> On 6 April 2015 at 19:02, Lieven Govaerts <lgo@mobsol.be> wrote:
>>>>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <bert@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

Mime
View raw message