kevinquinnyo / php-raid

General purpose RAID calculator and library in php
1 stars 3 forks source link

Take a look to my fork :) #33

Open tigerblue77 opened 5 years ago

tigerblue77 commented 5 years ago

Hi :)

I just forked your repository, could you take a look to my adds (I couldn't test it because I miss dependencies that are not documented and your paths in every file makes your code difficult to improve and I wouldn't change everything, just improve ^^...)

I'm sorry about a few things :

My objective is to include a module like this code on my personal web platform (that regroups some useful tools of all kinds)

Have a good day !

kevinquinnyo commented 5 years ago

@tigerblue77 I'll take a look later tonight if I can. Thanks!

kevinquinnyo commented 5 years ago

I didn't have much time to look at it, and to be honest I did not know about Synology Hybrid Raid (SHR) before today.

Is it correct to say that SHR is actually only applicable to RAID 1 or RAID 5?

And if so, should we extend those classes? kevinquinnyo\Raid\RaidOne and kevinquinnyo\Raid\RaidFive ?

Ex:

// Raid1SHR.php
namespace kevinquinnyo\Raid;

class Raid1SHR extends Raid1
{
}

// Raid5SHR.php
namespace kevinquinnyo\Raid;

class Raid5SHR extends Raid5
{
}

If you can help educate me a bit on this I'd love to help you land a pull request that supports it. I'll look into it on my own in the meantime. Let me know if I misunderstand -- I'm not exactly a hardware guy ;)

tigerblue77 commented 5 years ago

Hi,

thank you for your answer and for looking at my code. By my side I'm an engineering apprentice in systems, networks and IT infrastructure but I code I my free time and learn it at school.

Yes i think you misunderstand, I would love to explain to you, but my explanations may not be very clear so I give your the Synology's explanation : https://www.synology.com/en-global/knowledgebase/DSM/tutorial/Storage/What_is_Synology_Hybrid_RAID_SHR

Don't hesitate to let me know if you don't understand it, I'll explain how the SHR RAID works. In fact, it is a succession of RAID 5's that permit to considerably reduce the lost space when you have drives of different sizes. As a result, I think that one classe is the most appropriate choice.

Could you improve your code a bit to remove the "namespace" and "use" directives at the beginning of each file please ?

If I'm not mistaken, your code depends on 2 libraries that (I think) you should include in this repository. I don't know what their use is, but I think if you use them, they are useful. It would be breat to have all we need in the repository.

Don't hesitate to tell me if I'm wrong as your code is very clear and comprehensible ; I don't want to ruin it.

Thanks, Dylan

EDIT : - regrouping all dependancies in this repository will allow me to test my code easily :)

tigerblue77 commented 5 years ago

Hi, is it ok for you ? :)

kevinquinnyo commented 5 years ago

@tigerblue77 I'm really sorry I haven't had a chance to dig into this yet, but I definitely will.

In the meantime, I think there might be some confusion on how to use the library. You mentioned that you're having dependency issues? Did you run composer install? This library requires composer which you can download here.

tigerblue77 commented 5 years ago

Hi,

Sorry for the delay. I don't know "Composer" at all and I didn't know I had to install it to make it work. I will try in a Debian VM to be safe and I'll tell you :)

tigerblue77 commented 5 years ago

Hi,

sorry for the delay again, I taked a look to composer and installed it but I should have miss something as it don't work... Here is what I've done : `mkdir php-raid

cd php-raid

php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"

php -r "if (hash_file('sha384', 'composer-setup.php') === '48e3236262b34d30969dca3c37281b3b4bbe3221bda826ac6a9a62d6444cdb0dcd0615698a5cbe587c3f0fe57a54d8f5') { echo 'Installer verified'; } else { echo 'Installer corrupt'; unlink('composer-setup.php'); } echo PHP_EOL;"

php composer-setup.php

php -r "unlink('composer-setup.php');"

sudo mv composer.phar /usr/local/bin/composer

git clone https://github.com/kevinquinnyo/php-raid.git .

composer install`

But after that...

Where do I code my "Main" php file to test ? How do I run it ?

And at the beginnin of each of your files you have "namespace kevinquinnyo\Raid;". Do I have to change it ? Thanks by advance

kevinquinnyo commented 5 years ago

@tigerblue77 typically with a library like this you would be requiring it as a dependency in your own code by creating a composer.json of your own and listing it in the require section.

If you are just wanting to experiment with the library you can simply create a file in the base of the project directory like this:

<?php
require __DIR__ . '/vendor/autoload.php'; // this is what you are missing to autoload all the namespaces

use kevinquinnyo\Raid\Raid\RaidFive;
use kevinquinnyo\Raid\Drive;

$drives = [
    new Drive(1024, 'ssd', 1),
    new Drive(1024, 'ssd', 2),
    new Drive(1024, 'ssd', 3),
];
$raidFive = new RaidFive($drives);

var_dump($raidFive);
tigerblue77 commented 5 years ago

Hi ! :)

thanks to your good explanations, I succeeded to run the project and improve the raid SHR that is now working great :) (have a look ;))

I'll continue to improve your/the/our project ^^ you can merge if you want :)

kevinquinnyo commented 5 years ago

@tigerblue77 Nice work!

Can you open a pull request? You should see something like this when you do it:

2019-04-27-10-51-25_scrot

There are a few things that will need to be changed but you can go ahead and open the PR now:

  1. Replace Main.php with unit tests for the new SHR raid type. You can run tests with vendor/bin/phpunit from the main project directory. (composer install provides that). Look at the current structure of the tests and create a new RaidSHRTest.php like the other raid levels. Try to create a test for every public method.

  2. Run vendor/bin/phpcs -a (also provided by composer) and fix any code style issues that appear until it doesn't report any errors. (this will complain about things like spacing/indentation issues, etc)

I may also have a few other nitpicks after that but it looks good to me at first glance.

tigerblue77 commented 5 years ago

@kevinquinnyo thank you :)

Yes I'll do but... Wow... Composer is a good tool, thanks for your explanations but daaammnn... I prefer tabulations haha ! x)

Please have a look to the changes I made to your DriveTest.php file please. As in Drive.php I cast the size to an int (it seems logical and was creating problems with my SHR RAID), I've editted the test :)

It seems your RAID 5, 6 and 10 are not working, I'll investigate before pulling ;)

kevinquinnyo commented 5 years ago

:) Did you mean to close your pull request #34 ? I'll be glad to take a look at it and probably suggest some improvements once it's open.

tigerblue77 commented 5 years ago

Sorry I'm not sure to understand your question as English is not my main language ^^ I closed the pull request just because I found problems on my code that are solved now.

I'm a beginner in GIT/GitHub so don't hesitate to tell me if I do shit :P I'll open a new pull request :)

kevinquinnyo commented 5 years ago

@tigerblue77 No problem. Your english is fine.

You can just open the PR, and then push commits up to your branch, or you can fix whatever you need to and then submit the PR. It's really up to you.

I'll be glad to help if you have any problems. :)

tigerblue77 commented 5 years ago

@kevinquinnyo thank you :)

Well, I resolved the problems but there's still a problem with RAID 10... I have to think about the algorithm ^^' so I'll have a look when I'll have time :)

Thank you :) the PR is here https://github.com/kevinquinnyo/php-raid/pull/35

kevinquinnyo commented 5 years ago

@tigerblue77 thank you! I'll review this and report back soon

tigerblue77 commented 1 year ago

Hello @kevinquinnyo, any news ?