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.
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.
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.
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.
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?
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.
usr/src/cmd/clcommands/lib/libclcmds/Makefile.com
Same problem about using POST_S10_BUILD rather than tying in with the
WEAK_MEMBERSHIP macro.
usr/src/cmd/clcommands/lib/libclcmds/common/clnode/clnode_resolve_config.cc
Same point about NULL vs 0.
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."
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])?"
usr/src/head/scadmin/scconf.h
Here again you define WEAK_MEMBERSHIP. This definition should be one
place only.
usr/src/lib/libscconf/Makefile.com
Same problem about using POST_S10_BUILD.
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.
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
