search

Seagate / halon

High availability solution
Apache License 2.0
1 stars 0 forks source link

WIP: EOS-187: [multi-ios-hs1] data types, parseInitialData #1585

Closed mssawant closed 5 years ago

mssawant commented 5 years ago

Closes EOS-187

vvv commented 5 years ago

closed

vvv commented 5 years ago

Closing Halon merge requests.

vvv commented 5 years ago

I believe there is nothing else to be done in this patch. Good work, @mandar.sawant!

This patch cannot be landed without EOS-166 though, which I am yet to deliver. (I've added WIP: prefix to this merge request's title in order not to land it accidentally.)

vvv commented 5 years ago

resolved all discussions

vvv commented 5 years ago

added 1 commit

Compare with previous version

vvv commented 5 years ago

changed this line in version 4 of the diff

vvv commented 5 years ago

changed the description

vvv commented 5 years ago

marked as a Work In Progress

vvv commented 5 years ago

added 3 commits

Compare with previous version

vvv commented 5 years ago

Fixing this isn't worth the trouble.

vvv commented 5 years ago

It's concatMap. ;)

vvv commented 5 years ago

It's w x y z in the alphabet. Gotcha! :smiling_imp:

mssawant commented 5 years ago

added 1 commit

Compare with previous version

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

changed this line in version 2 of the diff

mssawant commented 5 years ago

May be tests. Once Halon changes are in place, need to make sure cluster bootstraps fine. It would also be great if we demo fail over where these changes would help.

mssawant commented 5 years ago

Done.

mssawant commented 5 years ago

Agree.

mssawant commented 5 years ago

Makes sense. Refactored.

mssawant commented 5 years ago

Agree, done.

mssawant commented 5 years ago

Removed.

mssawant commented 5 years ago

Agree, fixed.

mssawant commented 5 years ago

Yes, changed it because of the some build earlier which I probably fixed differently later. Fixed it.

mssawant commented 5 years ago

Agree, there's a possibility of duplicate M0Devices. Updated.

vvv commented 5 years ago

@mandar.sawant The patch looks good!

Apart from configuration files (m0genfacts, .ede templates, h0fabricator), what else is needed for the “Multiple IOS per node” feature to work?

vvv commented 5 years ago

Consider exporting host2devs as suggested above.

vvv commented 5 years ago

[optional] We could use TupleSections extension to reduce the boilerplate:

, mdsProcess ifaddr $ map (0,) [CST_MDS, CST_RMS, CST_ADDB2]
, m0t1fsProcess ifaddr [(0, CST_RMS)]
, iosProcess ifaddr $ (_id_drives, CST_IOS) : map (0,) [CST_RMS, CST_SNS_REP, CST_SNS_REB, CST_ADDB2, CST_ISCS]
vvv commented 5 years ago

Actually, don't bother introducing that IP4 data type. This can be done at leisure time, if we feel like refactoring. (He he, I've just condemned this data type to nonexistence.)

vvv commented 5 years ago

1) We underuse type system here. Suggestion:

-- | IPv4 octets.
data IP4 = IP4 Word8 Word8 Word8 Word8

2) The second tuple does not let us have meaningful arguments. (My first question was “Was does Int value signify?” I would not have this question if there was self-descriptive argument.) Suggestion:

mkSvc :: IP4 -> Endpoint -> Int -> ServiceType -> CI.M0Service
mkSvc (IP4 _ _ _ w) ep nrDrives svcType =

And this is how it would be used:

  , CI.m0p_services = map (uncurry $ mkSvc ifaddr ep) devs
vvv commented 5 years ago

This is equivalent to map host2devs. If host2devs was exported, we would not need hosts2devs at all.

vvv commented 5 years ago

These lines should not remain in the final patch. Do you want to keep them while the patch is being reviewed?

vvv commented 5 years ago

[optional] Explicit import lists or quantified imports are preferable. What other identifiers of Data.List were used? If there were not many of them, perhaps they could be added to the explicit import list?

vvv commented 5 years ago

Why is this needed? Data constructors of Either (Left and Right) exported by Prelude, no explicit import is necessary.

vvv commented 5 years ago

We might want to use Set.toList . Set.fromList . concat $ to prevent duplicates. Unless we are absolutely sure (I'm not) that no M0Device can ever be used by several services.