Re: Deadline of Github pull request for Hammer release (question)

[Loic Dachary <loic@dachary.org>]

[-- Attachment #1: Type: text/plain, Size: 14031 bytes --]

Hi,

On 22/01/2015 09:42, Miyamae, Takeshi wrote:
> Hi Loic,
> 
> Thanks for your additional comments.
> 
>> Would you be so kind as to update the https://github.com/t-miyamae/ceph/blob/master/src/test/erasure-code/Makefile.am
> 
> Sure. We'll do that.
> 
>> This is likely to fail on a slow machine. Instead of waiting you should use a lock.
> 
> The purpose of sleep(1) is to ensure sequentiality between threads, but it's not enough on slow
> machines as you mentioned. So, we'd like another example that uses a mutex lock. What files
> should we refer to?

src/test/common/test_sharedptr_registry.cc contains an example. The classes that are helpful in implementing thread safe are in src/common/Cond.h and src/common/Mutex.h

However, I'm not sure I understand why you need threads at all to test the plugin. Could you explain ?

> 
>> They however all have the same assert ( EXPECT_TRUE(shec->matrix == NULL); )
> 
> We agree that EINVAL is better than EIO on verifying input variables. We'll review our whole
> sources and fix both our plugin and test codes.

Thanks :-) What do you think of my comments (about minimum_to_decode_3 and minimum_to_decode_2 ) ?

Cheers

> 
> P.S. We are debugging thread safe mSHEC to release it tomorrow.
> 
> Best regards,
> Takeshi Miyamae
> 
> -----Original Message-----
> From: Loic Dachary [mailto:loic@dachary.org] 
> Sent: Wednesday, January 21, 2015 11:13 PM
> To: Miyamae, Takeshi/宮前 剛
> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾 鷹詔; Paul Von-Stamwitz (PVonStamwitz@us.fujitsu.com)
> Subject: Re: Deadline of Github pull request for Hammer release (question)
> 
> Hi,
> 
> I see them at https://github.com/t-miyamae/ceph/blob/master/src/test/erasure-code/TestErasureCodeShec.cc etc. thanks ! Would you be so kind as to update the https://github.com/t-miyamae/ceph/blob/master/src/test/erasure-code/Makefile.am to add the compilation instructions for these files ?
> 
> I see the tests includes a few threads (thread1 to thread5). It looks like they are used for measuring performance, am I right ? 
> 
> 	pthread_t tid;
> 	pthread_create(&tid,NULL,thread1,shec);
> 	sleep(1);
> 
> This is likely to fail on a slow machine. Instead of waiting you should use a lock. You will find examples in other tests, using various techniques depending on the context. If you'd like I could point to one. I'd have to better understand the intent of this test before I do.
> 
> From init_5 to init_22 there are various combinations of parameters which suggest checking for all kinds of errors (m being negative, or a string instead of a number etc.). They however all have the same assert ( EXPECT_TRUE(shec->matrix == NULL); ). It would be better to have an expectation that verifies the error has been caught as expected. Is it part of what your completing at the moment ?
> 
> minimum_to_decode_2 expectation for when it succeeds should be more precise that
> 
> 	EXPECT_TRUE(minimum_chunks.size());
> 
> since minimum_chunks is expecting to have a size that does not vary. And if it does that would be a problem.
> 
> In minimum_to_decode_3 after
> 
> 	EXPECT_NE(want_to_decode,minimum_chunks);
> 
> you could check the expected content of minimum_chunks also. Unless there is a random aspect to it ? 
> 
> Cheers
> 
> On 20/01/2015 10:07, Miyamae, Takeshi wrote:
>> Hi Loic,
>>
>> I'd appreciated your help very much.
>> I have just uploaded 2 test files into the fork repository.
>>
>> https://github.com/t-miyamae/ceph
>> files:
>>   src/test/erasure-code/TestErasureCodeShec.cc
>>   src/test/erasure-code/TestErasureCodeShec364.cc
>>
>> I'm sorry that tests for the thread safety codes has not been included yet.
>>
>> Thank you in advance,
>> Takeshi Miyamae
>>
>> -----Original Message-----
>> From: ceph-devel-owner@vger.kernel.org 
>> [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Loic Dachary
>> Sent: Tuesday, January 20, 2015 4:23 PM
>> To: Miyamae, Takeshi/宮前 剛
>> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾 鷹詔; 
>> Paul Von-Stamwitz (PVonStamwitz@us.fujitsu.com)
>> Subject: Re: Deadline of Github pull request for Hammer release 
>> (question)
>>
>> Hi,
>>
>> Thanks for the update. To speed up things, I could review what's already published. Did you add the tests already ?
>>
>> Cheers
>>
>> On 20/01/2015 01:48, Miyamae, Takeshi wrote:
>>> Hi Loic,
>>>
>>> Thank you for your advice which suggested providing SHEC as an extra-package.
>>> We believe it's generally a good idea, but we are hoping SHEC would 
>>> be merged into Hammer because that would have an apparent advantage 
>>> from a viewpoint of maintenance support.
>>>
>>> As for the thread safety issue, we totally agree with you and we are trying to solve it.
>>> We will complete it in a few days and we'd like to ask you to review 
>>> it again so that it might be in time for Hammer's feature freeze (v0.93).
>>>
>>> Best regards,
>>> Takeshi Miyamae
>>>
>>> -----Original Message-----
>>> From: ceph-devel-owner@vger.kernel.org 
>>> [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Loic Dachary
>>> Sent: Wednesday, January 14, 2015 3:03 AM
>>> To: Miyamae, Takeshi/宮前 剛
>>> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾 鷹詔;
>>> Paul Von-Stamwitz (PVonStamwitz@us.fujitsu.com)
>>> Subject: Re: Deadline of Github pull request for Hammer release
>>> (question)
>>>
>>> Hi,
>>>
>>> On 13/01/2015 18:05, Miyamae, Takeshi wrote:
>>>> Hi Loic,
>>>>
>>>> Thank you for your quick review.
>>>>
>>>>> Could you reference the jerasure files (they are in the jerasure 
>>>>> plugin already) instead of including them in your patch ?
>>>>
>>>> We had used the reference of jerasure v2.0 files before, but changed 
>>>> to including v1.2 files after the patent issue.
>>>
>>> If you are refering to http://erasure-code-patents.xyz/, it can safely be ignored.
>>>
>>>> However, we can restore the reference right away if we are needed.
>>>>
>>>>> In ::prepare you reuse the matrix, if possible. If your intention 
>>>>> is to improve performances, you should consider using the same approach as the isa plugin.
>>>>
>>>> We have found a performance issue caused by redundant 
>>>> initializations of the module, and solved it by caching the initialized variables in memory.
>>>> If that is the same approach as isa-plugin you mentioned, we also do 
>>>> not have the same issue any more.
>>>
>>> The ISA plugin approach is thread safe, that's the important part.
>>>
>>>>> I'll have more comments once you have unit / functional tests
>>>>
>>>> We do have the those unit/functional tests, but the server is unreachable at this moment.
>>>> Please let us upload them to the repository tomorrow.
>>>
>>> Great.
>>>
>>>>> Regarding your initial question: I think it is too late for the Hammer release.
>>>>
>>>> We are so disappointed to hear that, but we must apologize for the late request.
>>>> If there would be any chance that SHEC be pulled into hammer, we are 
>>>> very happy to conduct all the necessary tests by ourselves.
>>>> Please let us know the detailed schedule if this is a feasible option.
>>>
>>> Note that since this is a plugin it will be easy for someone to install it on a Hammer cluster, simply by copying the plugin files to each OSD / MON and restarting them. If there is some kind of urgency for you to be able to install this new plugin, I would advise making a package that contains just this file, restart the OSD once installed and recommend that the installation is done on all machines of the cluster (because every OSD / MON need to know about the plugin). 
>>>
>>> Cheers
>>>
>>>> Best regards,
>>>> Takeshi Miyamae
>>>>
>>>> -----Original Message-----
>>>> From: ceph-devel-owner@vger.kernel.org 
>>>> [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Loic Dachary
>>>> Sent: Tuesday, January 13, 2015 9:46 PM
>>>> To: Miyamae, Takeshi/宮前 剛
>>>> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾 
>>>> 鷹詔;
>>>> Paul Von-Stamwitz (PVonStamwitz@us.fujitsu.com)
>>>> Subject: Re: Deadline of Github pull request for Hammer release
>>>> (question)
>>>>
>>>> Hi,
>>>>
>>>> It's a great start :-) A few comments:
>>>>
>>>> Could you reference the jerasure files (they are in the jerasure plugin already) instead of including them in your patch ?
>>>>
>>>> In ::prepare you reuse the matrix, if possible. If your intention is 
>>>> to improve performances, you should consider using the same approach 
>>>> as the isa plugin. See 
>>>> http://workbench.dachary.org/ceph/ceph/blob/giant/src/erasure-code/i
>>>> s
>>>> a
>>>> /ErasureCodePluginIsa.cc#L36 which is and independent type 
>>>> http://workbench.dachary.org/ceph/ceph/blob/giant/src/erasure-code/i
>>>> s a /ErasureCodeIsaTableCache.h with only one instance and is 
>>>> populated as needed and protected by a lock to make it thread safe :
>>>> http://workbench.dachary.org/ceph/ceph/blob/giant/src/erasure-code/i
>>>> s
>>>> a
>>>> /ErasureCodeIsaTableCache.h#L101
>>>>
>>>> I'll have more comments once you have unit / functional tests ( similar to http://workbench.dachary.org/ceph/ceph/blob/giant/src/test/erasure-code/TestErasureCodeJerasure.cc ). 
>>>>
>>>> Regarding your initial question: I think it is too late for the Hammer release. But from what I read it looks like we'll be able to merge in the early stages of the next release and that will give us time to properly test it.
>>>>
>>>> Cheers
>>>>
>>>> On 13/01/2015 11:34, Miyamae, Takeshi wrote:
>>>>> Hi Loic,
>>>>>
>>>>> I'm so sorry. The following is the correct repository.
>>>>>
>>>>> https://github.com/t-miyamae/ceph
>>>>>
>>>>> Best regards,
>>>>> Takeshi Miyamae
>>>>>
>>>>> -----Original Message-----
>>>>> From: Loic Dachary [mailto:loic@dachary.org]
>>>>> Sent: Tuesday, January 13, 2015 7:26 PM
>>>>> To: Miyamae, Takeshi/宮前 剛
>>>>> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾
>>>>> 鷹詔;
>>>>> Paul Von-Stamwitz (PVonStamwitz@us.fujitsu.com)
>>>>> Subject: Re: Deadline of Github pull request for Hammer release
>>>>> (question)
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 13/01/2015 11:24, Miyamae, Takeshi wrote:
>>>>>> Hi Loic,
>>>>>>
>>>>>>> Although we're late in the Hammer roadmap, it's a good time for an early preview. It will help show what needs to be changed to accomodate the SHEC plugin.
>>>>>>
>>>>>> Thank you for your advices.
>>>>>> We have uploaded our latest codes to the following folk repository for an early review.
>>>>>>
>>>>>> https://github.com/miyamae-takeshi/multiple-shec
>>>>>
>>>>> It's 404 ? Is it a private repository maybe ?
>>>>>
>>>>>>
>>>>>> SHEC is located in src/erasure-code/shec directory.
>>>>>>
>>>>>> We are verifying SHEC's advantages on our Ceph cluster. It takes a little bit more.
>>>>>> Would you please start the review before that?
>>>>>>
>>>>>> Best regards,
>>>>>> Takeshi Miyamae
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ceph-devel-owner@vger.kernel.org 
>>>>>> [mailto:ceph-devel-owner@vger.kernel.org] On Behalf Of Loic 
>>>>>> Dachary
>>>>>> Sent: Wednesday, January 7, 2015 12:52 AM
>>>>>> To: Miyamae, Takeshi/宮前 剛
>>>>>> Cc: Ceph Development; Shiozawa, Kensuke/塩沢 賢輔; Nakao, Takanori/中尾
>>>>>> 鷹詔
>>>>>> Subject: Re: Deadline of Github pull request for Hammer release
>>>>>> (question)
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 06/01/2015 12:49, Miyamae, Takeshi wrote:
>>>>>>> Dear Loic,
>>>>>>>
>>>>>>> I'm Takeshi Miyamae, one of the authors of SHEC's blueprint.
>>>>>>>
>>>>>>> Shingled Erasure Code (SHEC)
>>>>>>> https://wiki.ceph.com/Planning/Blueprints/Hammer/Shingled_Erasure
>>>>>>> _
>>>>>>> C
>>>>>>> o
>>>>>>> d
>>>>>>> e
>>>>>>> _(SHEC)
>>>>>>
>>>>>> The work you have done is quite impressive :-)
>>>>>>
>>>>>>> We have revised our blueprint shown in the last CDS to extend our 
>>>>>>> erasure code layouts and describe the guideline for choosing SHEC among various EC plugins.
>>>>>>> We believe the blueprint now answers all the comments given at the CDS.
>>>>>>
>>>>>> Great.
>>>>>>
>>>>>>> In addition, we would like to ask for your advice on the schedule 
>>>>>>> of our github pull request. More specifically, we would like to 
>>>>>>> know its deadline for Hammer release.
>>>>>>> (As we have not really completed our verification of SHEC, we are 
>>>>>>> wondering if we should make it open for early preview.)
>>>>>>
>>>>>> Although we're late in the Hammer roadmap, it's a good time for an early preview. It will help show what needs to be changed to accomodate the SHEC plugin.
>>>>>>
>>>>>> Cheers
>>>>>>
>>>>>>> Thank you in advance,
>>>>>>> Takeshi Miyamae
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" 
>>>>>>> in the body of a message to majordomo@vger.kernel.org More 
>>>>>>> majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Loïc Dachary, Artisan Logiciel Libre
>>>>>>
>>>>>
>>>>> --
>>>>> Loïc Dachary, Artisan Logiciel Libre
>>>>>
>>>>
>>>> --
>>>> Loïc Dachary, Artisan Logiciel Libre
>>>>
>>>
>>> --
>>> Loïc Dachary, Artisan Logiciel Libre
>>>
>>
>> --
>> Loïc Dachary, Artisan Logiciel Libre
>>
>> N     r  y   b X  ǧv ^ )޺{.n +   z ]z   {ay \x1dʇڙ ,j   f   h   z \x1e w   
>    j:+v   w j m         zZ+     ݢj"  !tml=
>>
> 
> --
> Loïc Dachary, Artisan Logiciel Libre
> 
> N�����r��y���b�X��ǧv�^�)޺{.n�+���z�]z���{ay�\x1dʇڙ�,j\a��f���h���z�\x1e�w���\f���j:+v���w�j�m����\a����zZ+�����ݢj"��!tml=
> 

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]