Hi Glen,
On 2/22/07, Glen Daniels <[EMAIL PROTECTED]> wrote:
Hey folks:
David Illsley wrote:
> Here's a diif of the branch for those who are interested.
Thanks for the summary David!
I took a quick look at the diff and have a few comments for the folks
working on this stuff. I haven't yet fully grokked the implications of
all this, so there may be a few more architectural comments forthcoming
after a more detailed read (and after I finish tuning my head all the
way back in to Axis2!).
* Can we please remove all message strings from the code itself and
resource-ize them?
* Noticed a few code cleanliness issues like overly long lines and
missing space around operators... maybe run this through someone's
IDE code-cleanup?
I am doing this. Will make sure this is done before the commit.
* Rather than continually repeating
context.setClustered(true);
clusterManager.addContext(context);
why not just have clusterManager.addContext() be responsible for
setting the clustered flag on the context? Cleaner code, and
you won't have potentially incorrectly set clustered flags in
case of errors.
OK.
* Take a look at this code in AbstractContext:
> + public void flush () throws AxisFault {
> + //if clustering is enabled, ClusterManager will be called to
replicate the context state.
> + if (clustered) {
> +
> + ClusterManager clusterManager = null;
> +
> + if (this instanceof ConfigurationContext) {
> + ConfigurationContext configurationContext =
(ConfigurationContext) this;
> + clusterManager =
configurationContext.getAxisConfiguration().getClusterManager();
> +
> + } else if (this instanceof ServiceGroupContext) {
> + ConfigurationContext configurationContext =
(ConfigurationContext) this.getParent();
> + if (configurationContext==null) {
> + String message = "The parent of the
ServiceGroupContext has not been set";
> + throw new AxisFault (message);
> + }
> +
> + clusterManager =
configurationContext.getAxisConfiguration().getClusterManager();
> +
> + } else if (this instanceof ServiceContext) {
> + ServiceGroupContext serviceGroupContext =
(ServiceGroupContext) this.getParent();
> + if (serviceGroupContext==null) {
> + String message = "The parent of
the ServiceContext has not been set";
> + throw new AxisFault (message);
> + }
> +
> + ConfigurationContext configurationContext =
(ConfigurationContext) serviceGroupContext.getParent();
> + if (serviceGroupContext==null) {
> + String message = "The parent of
the ServiceGroupContext has not been set";
> + throw new AxisFault (message);
> + }
> +
> + clusterManager =
configurationContext.getAxisConfiguration().getClusterManager();
> + }
This kind of "if (this instanceof SubClass)" structure is usually
a signal that there's some more elegant way to get the same job done.
In this case, I'd suggest either a) just put the ClusterManager
member in the AbstractContext itself, or (even better) introduce
a "Clusterable" interface which contains the getClusterManager()
method. Either of these solutions allows extensibility and makes
the above code much cleaner.
More later...
The reason for this was not having a abstract method to get the
ConfigurationContext. I added a getRootContext() abstract method to the
AbstractContext which will do the trick.
Thanks a lot for the info.
Chamikara
Thanks,
--Glen
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
