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 ?
> 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" ?
> 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" ?
> 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
>
