synapse-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hubert, Eric" <Eric.Hub...@foxmobile.com>
Subject RE: [VOTE] Release Synapse-2.0.0 (Take2)
Date Thu, 16 Dec 2010 13:02:55 GMT
Hi Ruwan,

there is not much to worry about. I have been able to rather easily fix all of those issues.
I have been only a bit concerned about users which are not familiar with the Synapse code
at all.

As you said, the errors regarding SynapseArtifact.getDescription() came from custom mediator
implementations directly implementing the mediator interface without using the abstract base
implementation. I do not know how often people might have done this. For some of our mediators
there was no need to extend from the base implementation as it offered nothing meaningful
for those cases. I guess with Synapse 2.0 users are now strongly encouraged to extend the
base implementation. Otherwise they have to implement some aspects they do not like to care
about at all.

Thanks for the explanation regarding the properties usage. Now it’s purpose is clear. Just
from looking at the API and Unit-Tests (which is mostly a good start to understand something)
I wasn’t able to get it.

Thanks,
   Eric


________________________________
From: Ruwan Linton [mailto:ruwan.linton@gmail.com]
Sent: Thursday, December 16, 2010 12:42 PM
To: dev@synapse.apache.org
Subject: Re: [VOTE] Release Synapse-2.0.0 (Take2)

Hi Eric,

Going through your documentation and trying to document/fix them... here are my comments/explanations.
:-)
On Fri, Dec 10, 2010 at 1:11 PM, Hubert, Eric <Eric.Hubert@foxmobile.com<mailto:Eric.Hubert@foxmobile.com>>
wrote:
I found quite a bit of time to have a more detailed look at the incompatible changes and start
with an upgrade process - comments inline.
________________________________
From: Hubert, Eric [mailto:Eric.Hubert@foxmobile.com<mailto:Eric.Hubert@foxmobile.com>]
Sent: Thursday, December 09, 2010 12:40 PM

To: dev@synapse.apache.org<mailto:dev@synapse.apache.org>
Subject: RE: [VOTE] Release Synapse-2.0.0 (Take2)

Here is a list of incompatibilities immediately found. Some are only from unit tests – not
all will be considered as public API. To solve some tasks access was needed for different
reasons…
Somehow sorted by importance:

The type XYZ must implement the inherited abstract method AbstractMediatorFactory.createSpecificMediator(OMElement,
Properties)

This has been documented on the upgrading guide

The type XYZ must implement the inherited abstract method SynapseArtifact.getDescription()
The type XYZ must implement the inherited abstract method SynapseArtifact.setDescription(String)

From where did you get these errors, you cannot get these errors if you are extending from
the abstract classes, since it contains these implementations, and the description is automatically
available for the programmer through the abstract class. This is one of the main ideas of
the above API refactoring too.

Cannot override the final method from AbstractMediatorSerializer

This is caused by the serializeSpecificMediator introduction just as in the factory, and you
should use that instead of the final method. And yes this has been documented too.

The field AbstractMediatorFactory.log is not visible

This seems to be mistakenly changed by Hiranya to package private from protected, I have reverted
it to be protected so that you can use it now, sorry for the inconveniences.

The method createMediator(OMElement, Properties) in the type AbstractMediatorFactory is not
applicable for the arguments (OMElement)
The method createMediator(OMElement, Properties) in the type MediatorFactory is not applicable
for the arguments (OMElement)
The method createMediator(OMElement) of type XYZ must override or implement a supertype method

The above 3 cases has already been taken into account in the documentation of AbstractMediatorFactory/Serializer
change.


All of the above is what I assume could/should be considered public API and all of the changes
required to make custom mediator code compile again are trivial and quickly done. I did this
for a couple of mediators.
I have still one question: What has been the reason to add the Properties to the createSpecificMediator
method in the AbstractMediatorFactory? I tried to answer this question myself and looked for
usage of the properties in the source. I only checked a couple of implementation, but could
nowhere find a use. Everywhere this parameter was ignored in the Factory implementations and
callers passed in an empty Properties-Map.

This is to resolve the coupling between the server startup with Axis2 integration and Synapse
configuration building. Those are two concerns and earlier the ServerConfigurationInformation
class was a singleton and the rest of the synapse core code has mis used this singleton behavior
to get the access to information like SYNAPSE_HOME and RESOLVE_ROOT. These information really
are a set of arguments that needs to be passed into the configuration building framework,
but I thought ahead a little bit about what if we have more parameters in the future, and
introduced a generic properties map.

See the Axis2SynapseController#createSynapseConfiguration method to see how we pass in these
parameters to the configuration building framework removing the coupling.



The method getMediator(OMElement, Properties) in the type MediatorFactoryFinder is not applicable
for the arguments (OMElement)
The method getMediator(OMElement, Properties) in the type MediatorFactoryFinder is not applicable
for the arguments (OMElement)

The method getInstance() is undefined for the type ServerManager
DataSourceInformation cannot be resolved
DataSourceRegistrar cannot be resolved
JNDIBasedDataSourceRegistry cannot be resolved
MiscellaneousUtil cannot be resolved
RMIRegistryController
The import org.apache.synapse.util.datasource
The import org.apache.synapse.util.MiscellaneousUtil
The import org.apache.synapse.util.RMIRegistryController

These are documented generally under the API changes section.

I am yet to carefully go through your other suggestions on the API changes, let me get back
to you on that ASAP.

Thanks,
Ruwan


All the above seems to be stuff which had never been designed for external usage. It should
not be hard to figure out how to properly replace it. All but one compile errors are from
test/mock code used for verification of correctness of mediator implementations (including
factories/serializers).

If you guys are not in hurry tomorrow I would like to:
-       fix the remaining issues in our custom code
-       use a fixed version of the migration tool to migrate a large configuration file (for
my initial test I used something really small)
-       do some runtime mediation tests

Unfortunately I will not find time today.

Regards,
   Eric


________________________________
From: Hubert, Eric [mailto:Eric.Hubert@foxmobile.com<mailto:Eric.Hubert@foxmobile.com>]
Sent: Thursday, December 09, 2010 11:44 AM
To: dev@synapse.apache.org<mailto:dev@synapse.apache.org>
Subject: RE: [VOTE] Release Synapse-2.0.0 (Take2)

Answers inline. Not every statement was meant to describe a „problem“, I simply always
described what I did and what happened. Unexpected behaviour is separately mentioned…
________________________________



1) Synapse startup test with custom 1.2 config

- config has been put into repository/conf/synapse-config as it seems to got ignored in repository/conf

I think this is by design and we need to document this on the Upgrading guide.
Agreed.


- Synapse does not startup due to a problem in the config (e.g. missing registry implementation
class on the classpath)

I will have a look at this, I guess this needs to be fixed if there is an issue, can you please
give me a small config bit to re-produce this issue? may be you are using a WSO2 ESB registry
class shipped with WSO2 which of course is not available in synapse. :-(
Yes, my aim was to test a failure case. So it was absolutely expected that this fails. No
issue at all!


- Unexpected behaviour: Although Synapse detects the problems, tries to perform a clean shutdown,
it “keeps hanging” and does not return to the shell with an error return value

Can you please attach the log for this and steps to re-produce, this again I would like to
fix depending on the complexity of the issue... and if this gets slipped from 2.0.0 I suggest
immediately spinning a 2.0.1 to fix this and any other this sort of issues from the 2.0.0
branch. WDYT?
Yes, I personally do not consider this critical – it is simply only not nice. By the way,
the same condition can be reached with many different issues in a user’s config. Reproduction
is easy. Just specify any class name non existing in the classpath e.g. as the registry provider
implementation.



2) Migration Tool

- executing the migration tool expects a config in repository/conf (former config location)

If you type help for the migration tool sh you will find that it is the default location the
script looks for but you can specify your own location too.
Maybe it would be slightly better if an no arg execution outputs a usage instead of assuming
the default location and immediately starting to do something. But this is obviously a matter
of taste…


- old copy copied there and restarted

- migration tool modifies config

- Unexpected behaviour:

- after migration config stays in repository/conf and needs to be manually copied to repository/conf/synapse-conf
to be recognized

This again is the default location, assuming that you are running the migration tool for an
old configuration, but you could of course give the new location to be saved after migrating
the configuration. I guess we need a little bit more documentation around the migration tool.


- migration tool mistakenly removes namespaces (destroying the config), e.g.
    <code xmlns:tns="http://www.w3.org/2003/05/soap-envelope" value="tns:Receiver"/>
--> <syn:code value="tns:Receiver" /> resulting in startup issues

This we need to fix for this release, I will work on this.
Great, I’d also consider this to be a blocker as the migration tool can convert a working
config into a non-working one. I have not checked whether a backup is saved somewhere…


- migration tool removes indentation at the beginning of each xml element

This is a known issue, but I agree needs to be fixed, since it is not critical I would live
with this for the 2.0.0, but definitely a candidate for the next version, so we need to raise
an issue ticket on the synapse JIRA for this.
 Agreed, if no one beats me I can do this later today…




3) Traditional config in single file versus multi file configuration

- Unexpected behaviour:

  - replacing dummy synapse.xml with old (converted) config is not enough, it results in errors
if main and/or fault sequences are used (as the must exist only once), sequence files need
to be removed

from subdirectories

Yes, this I agree with, but cannot do much I guess again need to explain this on the Upgrading
guide
Is the reason for this, that no concept has precedence over the other? One can mix both approaches
as desired? If so I fully understand, but documentation is the least we should do. I would
also vote for a prominent link to the Upgrading section in the documentation right from the
front page. May under what’s new or so…



4) Usage of custom mediators / Site Documentation “Upgrading”

- In the docu I could not quickly locate a document summarizing the steps which need to be
done to upgrade custom mediators according to API changes (I received some AbstractMethodError).
I did not check the mediator sources against the current libraries to find out what is no
longer compiling…. Anyway a quick summary of API changes would be nice. As long as I haven’t
check the points which do not compile I cannot say for sure, whether those problems are due
to the fact that public methods not considered to be part of the public API have been used
on our end (which is not unlikely at all).

I agree, will try to add more on the API changes, at least for the mediator API.
 Great.




________________________________
From: Ruwan Linton [mailto:ruwan.linton@gmail.com<mailto:ruwan.linton@gmail.com>]
Sent: Thursday, December 09, 2010 7:04 AM

To: dev@synapse.apache.org<mailto:dev@synapse.apache.org>
Subject: Re: [VOTE] Release Synapse-2.0.0 (Take2)

So the Sandesha module can be easily removed if you are not doing any reliable messaging stuff.
Just need to delete the file from the repository/modules directory. I would even remove this
error message from the custom build of Sandesha2 as we any way seem to go for the 3rd round
of voting. :-)

Eric, I would like to wait for your feedback to do the 3rd RC. So take your time, but report
us any critical issue as soon as you get to them. May not need to be the complete list, you
can report them one by one as and when you find those, so that we can fix them if needs to
be and be ready for your next round of the feedback. BTW: must say that we really appreciate
your feedback.

Thanks,
Ruwan



--
Ruwan Linton
Software Architect & Product Manager, WSO2 ESB; http://wso2.org/esb
WSO2 Inc.; http://wso2.org

Lean . Enterprise . Middleware

phone: +1 408 754 7388 ext 51789
email: ruwan@wso2.com<mailto:ruwan@wso2.com>; cell: +94 77 341 3097
blog: http://blog.ruwan.org
linkedin: http://www.linkedin.com/in/ruwanlinton
google: http://www.google.com/profiles/ruwan.linton
tweet: http://twitter.com/ruwanlinton



--
Ruwan Linton
Software Architect & Product Manager, WSO2 ESB; http://wso2.org/esb
WSO2 Inc.; http://wso2.org

Lean . Enterprise . Middleware

phone: +1 408 754 7388 ext 51789
email: ruwan@wso2.com<mailto:ruwan@wso2.com>; cell: +94 77 341 3097
blog: http://blog.ruwan.org
linkedin: http://www.linkedin.com/in/ruwanlinton
google: http://www.google.com/profiles/ruwan.linton
tweet: http://twitter.com/ruwanlinton
Mime
View raw message