[Gluster-devel] answer_list in EC xlator

[fanghuang.data at yahoo.com]

On Wednesday, 3 June 2015, 15:22, Xavier Hernandez <xhernandez at datalab.es> wrote:


>
>
>Hi,
>
>On 06/03/2015 05:40 AM, Pranith Kumar Karampuri wrote:
>>
>>
>> On 06/02/2015 08:08 PM, fanghuang.data at yahoo.com wrote:
>>> Hi all,
>>>
>>> As I reading the source codes of EC xlator, I am confused by the
>>> cbk_list and answer_list defined in struct _ec_fop_data. Why do we
>>> need two lists to combine the results of callback?
>>>
>>> Especially for the answer_list, it is initialized
>>> in ec_fop_data_allocate, then the nodes are added
>>> in ec_cbk_data_allocate. Without being any accessed during the
>>> lifetime of fop, the whole list finally is released in ec_fop_cleanup.
>>> Do I miss something for the answer_list?
>> +Xavi.
>>
>> hi,
>>      The only reason I found is that It is easier to cleanup cbks using
>> answers_list. You can check ec_fop_cleanup() function on latest master
>> to check how this is.
>
>You are right. Currently answer_list is only used to cleanup all cbks 
>received while cbk_list is used to track groups of consistent answers. 
>Although currently doesn't happen, if error coercing or special 
>attribute handling are implemented, it could be possible that one cbk 
>gets referenced more than once in cbk_list, making answer_list 
>absolutely necessary.
>

That's a good point to put all the cbks into one group and put those with
consistent answers into the other group. But this designing policy cannot 
be understood easily from the comments, source codes or the list names (cbk_list,
answer_list). Could we rename the cbk_list to consist_list or something else 
easier to be followed?

>
>> Combining of cbks is a bit involved until you
>> understand it but once you do, it is amazing. I tried to add comments
>> for this part of code and sent a patch, but we forgot to merge it :-)
>> http://review.gluster.org/9982. If you think we can add more
>> comments/change this part of code in a way it makes it easier, let us
>> know. We would love your feedback :-). Wait for Xavi's response as well.
>>

This patch is much clearer. For the function ec_combine_update_groups,
since we only operate on one list, should we use ec_combine_update_group? The
word "groups" is confusing for readers who may think there are two or more groups.

--
Fang Huang