Hi Nick,
Thanks for you comments. Please find my response inline ...
Nicholas Solter wrote:
> Ganesh,
>
> A few comments below.
>
> Ganesh Ram N wrote:
>
>> Hi Nick,
>>
>> Thanks a lot for the quick review. Please find my response inline ...
>>
>> Nicholas Solter wrote:
>>
>>> Ganesh,
>>>
>>> Here are a few comments. I should note that I'm not too familiar with
>>> either the CLI or the weak membership code.
>>>
>>> General: You need to change copyrights to 2009 now.
>>>
>>>
>> I will do this change right at the end after I have incorporated all the
>> comments.
>>
>>> usr/src/cmd/clcommands/commands/clnode/Makefile
>>>
>>> Because weak membership is not dependent on OpenSolaris, we decided to
>>> control weak membership through a separate flag not inherently tied to
>>> the version. Note that the core weak membership implementation defines
>>> WEAK_MEMBERSHIP in usr/src/common/cl/sys/clconf_int.h
>>>
>>> This should be tied to the control of the CLI portions as well. So
>>> instead of using POST_S10_BUILD in the Makefile here, I think you'll
>>> need to define a new macro in some Makefile and use it to specify the
>>> WEAK_MEMBERSHIP definition that clconf_int.h can pick up.
>>>
>>>
>>>
>> As per Sambit's response on a different email, I have included
>> <sys/clconf_int.h> which has the definition for the WEAK_MEMBERSHIP
>> macro for all the C/C++ files. Similarly can you or anyone else point me
>> to a Makefile macro which I can include and use ?
>>
>
>
> We discussed this offline.
>
>
Thanks for your inputs..
>>> usr/src/cmd/clcommands/commands/clnode/clnode_cmd.h
>>>
>>> 64 #define CLNODE_SUBCMD_RESOLVE_CONFIG \
>>> 65 GETTEXT("Resolves the cluster weak membership during split
>>> brain")
>>>
>>> The rest of the messages seem to be imperative rather than descriptive,
>>> so this one should probably be, "Resolve the..." instead of "Resolves
>>> the..."
>>>
>>> Also, I don't think the description is quite right. This doesn't resolve
>>> "weak membership", it resolves the CCR configuration changes that
>>> occurred during weak membership. And it's not run during split brain,
>>> rather after the cluster is back together.
>>>
>>>
>>>
>> Lisa commented on the same text and we modified it to "Resolve split
>> brain in a weak-membership cluster".
>> Should this be modified to "Resolve cluster configuration conflicts in a
>> weak-membership cluster" ?
>>
>
> Yes, I prefer "cluster configuration conflicts".
>
> The point I think I was trying to make about "split-brain" was that this
> command is run after the split-brain has been resolved. So just changing
> "split-brain" to "weak-membership" I don't think really helps. I'd
> prefer something like, "Resolve cluster configuration conflicts that
> occurred during a split-brain." Or something like that.
>
>
I guess the subcommand name is generic hence we should put it as
"Resolve cluster configuration conflicts"
without the reference to split-brain.
Would this work ?
>>> usr/src/cmd/clcommands/commands/clnode/clnode_resolve_config_options.cc
>>>
>>> Pointers such as cl_option, node_name, and so on should be assigned NULL
>>> by default, not 0 casted to a pointer.
>>>
>>>
>>>
>> done
>>
>>> usr/src/cmd/clcommands/commands/clnode/clnodeinit_constants.cc
>>>
>>> I think we may have discussed this, but I can't remember why you need to
>>> specify the local node in the resolve-config command. Isn't it obvious
>>> based on the node on which you're running the command?
>>>
>>>
>>>
>> In New CLI, if there are no operands specified it equates to a WILD_CARD
>> operand which will match all the existing objects of that type. Hence
>> the requirement for the user to specify the node name as the operand
>> explicitly.
>>
>>> usr/src/cmd/clcommands/include/ClShow.h
>>> usr/src/cmd/clcommands/include/ClStatus.h
>>> usr/src/cmd/clcommands/include/clcommands.h
>>>
>>> I don't like these redundant definitions of WEAK_MEMBERSHIP with each
>>> other and with the definition in clconf_int.h. The macro should be
>>> defined in one place only.
>>>
>>>
>>>
>> Removed the macro from all of the above files and instead I have
>> included <sys/clconf_int.h> which has the definition for WEAK_MEMBERSHIP
>> macro.
>>
>>> usr/src/cmd/clcommands/lib/libclcmds/Makefile.com
>>>
>>> Same problem about using POST_S10_BUILD rather than tying in with the
>>> WEAK_MEMBERSHIP macro.
>>>
>>>
>>>
>> See comment above.
>>
>>> usr/src/cmd/clcommands/lib/libclcmds/common/clnode/clnode_resolve_config.cc
>>>
>>> Same point about NULL vs 0.
>>>
>>>
>>>
>> Done.
>>
>>> Why do you use the WEAK_MEMBERSHIP macro for some portions of this file?
>>> Isn't the whole file only for weak membership?
>>>
>>> usr/src/cmd/clcommands/lib/libclcmds/common/clquorum/clquorum_set.cc
>>>
>>> "This action might result in DATA LOSS !!!\"
>>>
>>> This message is a little extreme. I don't think we need even one, let
>>> alone three, exclamation marks. Also, I thought we'd agreed to say "data
>>> corruption or loss."
>>>
>>>
>>>
>> :) Should this be "This action might result in data corruption or loss\n" ?
>>
>
> Yes, that's better.
>
>
Thanks,
Ganesh Ram
> Thanks,
> Nick
>
>
>>> 387 gettext("Are you sure you want to enable
>>> multiple "
>>> 388 "partitions in the cluster to be
>>> operational. "
>>> 389 "(y/[n])?"));
>>>
>>>
>>> Shouldn't have a period before the "(y/[n])?"
>>>
>>>
>>>
>> I have removed the period as per Lisa's comments earlier.
>>
>>> usr/src/head/scadmin/scconf.h
>>>
>>> Here again you define WEAK_MEMBERSHIP. This definition should be one
>>> place only.
>>>
>>>
>>>
>> Agreed.
>>
>>> usr/src/lib/libscconf/Makefile.com
>>>
>>> Same problem about using POST_S10_BUILD.
>>>
>>>
>> Agreed
>>
>>> usr/src/lib/libscconf/common/scconf_infrafile.c
>>>
>>>
>>> 114 *
>>> 115 * SOL_VERSION >= s11
>>> 116 * scconf_cluster_multi_partitions
>>> 117 * scconf_cluster_ping_targets
>>> 118 * END SOL_VERSION >= s11
>>>
>>> I know this is just comments, but you should use WEAK_MEMBERSHIP, not
>>> SOL_VERSION here.
>>>
>>>
>>>
>> Yes. I will do so.
>>
>> Thanks,
>> Ganesh Ram
>>
>>> Thanks,
>>> Nick
>>>
>>> Ganesh Ram Nagarajan wrote:
>>>
>>>
>>>> Hi All,
>>>>
>>>> Please review our CLI changes for supporting Weak Membership Model
>>>>
>>>> Webrev Available @ http://cr.opensolaris.org/~gnr259/colorado-cli/webrev/
>>>>
>>>> Summary of changes :
>>>> ===============
>>>>
>>>> New Quorum object "global_quorum"
>>>> Used for specifying the "ping_targets" property [list of
>>>> ping_targets can be added]
>>>> Used for specifying the "multiple_partitions" property
>>>> True - Specifies we are in weak membership
>>>> False - Specifies we are in strong membership
>>>>
>>>> New subcommand for clnode command "resolve-config"
>>>> Used for declaring the winner node after split-brain (Runs in both
>>>> cluster & non-cluster mode)
>>>> Used for declaring the loser node after split-brain (Run only in
>>>> non-cluster mode)
>>>> clnode status command used for querying for the presence of
>>>> unresolved CCR changes
>>>>
>>>> Regards & Thanks,
>>>> Ganesh Ram & Nandan
>>>>
>>>>
>>>> _______________________________________________
>>>> ha-clusters-discuss mailing list
>>>> ha-clusters-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>>>>
>>>>
>>> _______________________________________________
>>> ha-clusters-discuss mailing list
>>> ha-clusters-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>>>
>>>
>> _______________________________________________
>> ha-clusters-discuss mailing list
>> ha-clusters-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>>
>
> _______________________________________________
> ha-clusters-discuss mailing list
> ha-clusters-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>
