Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MessageImpl changed to set the header part if a name is null #1

Merged
merged 1 commit into from
Jun 22, 2012

Conversation

ibek
Copy link
Contributor

@ibek ibek commented Jun 19, 2012

This change is needed for the SWITCHYARD-703.

@jeffyu jeffyu merged this pull request into riftsaw:master Jun 22, 2012
@jeffyu
Copy link
Contributor

jeffyu commented Jun 22, 2012

Hi Ivo,

Thanks a lot for your contribution.

It would be great if you rebase the code when you make the pull request next time. :). This time, as your updated branch is not based on the latest code, I couldn't merge it. But I reviewed the code, it is a small change, therefore I used the github built-in function of merging pull request.

Regards
Jeff

@jeffyu
Copy link
Contributor

jeffyu commented Jun 22, 2012

Hi Ivo,

Sorry for re-opening this issue, however, on applying your patch to the RiftSaw, found that this fix seem to break the unit tests of the RiftSaw. Therefore re-open it. :-(

If you can rebase the riftsaw-ode codebase, and then build it with this update, when you run the build for riftsaw, you would see the failed test cases.

Could you please take a look at it to see if they are any updates that might affect your update, or your update might not work as you thought with latest code?

Thanks
Jeff

@ibek
Copy link
Contributor Author

ibek commented Jun 22, 2012

Hi Jeff

I will investigate it. It's weird because there is almost nothing to be broken. :)

@jeffyu
Copy link
Contributor

jeffyu commented Jun 22, 2012

I was thinking of the same, so must be some minor thing there.

Just to let you know, I also revert your update from the repo for now.

Regards
Jeff

@ibek
Copy link
Contributor Author

ibek commented Jun 22, 2012

So, I pulled from upstream and I successfully built it without any failures or errors in unit tests. Maybe something bad happened with merging. However if you have a moment you can try it here first https://github.com/ibek/riftsaw-ode/tree/SWITCHYARD-703

Should I create new pull request?

@jeffyu
Copy link
Contributor

jeffyu commented Jun 25, 2012

Hi Ivo,

Just to quick check. I am saying that the fix would break the RiftSaw project, not the RiftSaw-ODE project itself.

Regards
Jeff

@ibek
Copy link
Contributor Author

ibek commented Jun 25, 2012

Hi Jeff,

thank you, I thought the tests in riftsaw-ode, don't know why ... it was friday :)
finally I reproduced it with current riftsaw master branch... do you think "testSimpleCorrelationVersioned(org.riftsaw.engine.BPELEngineTest): Failed: org.apache.ode.bpel.iapi.ContextException: Deploy failed; Deployment Unit "simple_correlation_HelloGoodbye-1" already deployed!"?

After I change the branch to SWITCHYARD-703 in RiftSaw I don't have this issue. It worked even for master but in the version before 4 months https://github.com/ibek/riftsaw . So, I would try to merge pull request for riftsaw-ode and simultaneously pull request for riftsaw.

Thanks, Ivo

@jeffyu
Copy link
Contributor

jeffyu commented Jun 25, 2012

Hi Ivo,

Yes, I got the same error, but the root cause for this 'already deployed' was because the previous error. If you look into the failed test case, that would be great.

Regards
Jeff

@ibek
Copy link
Contributor Author

ibek commented Jun 25, 2012

I slightly edited the BPELEngineTest to undeploy processes properly but I'm not sure if it will help because I had the exception only once and I cannot reproduce it :(
The changes are here ibek/riftsaw@db7392f

@jeffyu
Copy link
Contributor

jeffyu commented Jun 26, 2012

Thanks Ivo for the update.

Was curious why it was causing the undeployment throwing the error. Your fix seem to catch the error and then ignore it, please let me know if I misunderstood something here. I'll take a look at it tomorrow and then get back to you.

Regards
Jeff

@ibek
Copy link
Contributor Author

ibek commented Jun 28, 2012

Hi Jeff,

I think that a process should be undeployed even if a test fails because otherwise it causes failures somewhere else. But yes, I probably ignore the error.

BTW, I found another way to put pull request into upstream ... here (the Patch and Apply - https://help.github.com/articles/using-pull-requests) you can find how to use patch instead of merge and the commits don't have to be based on latest code ;)

Regards, Ivo

@jeffyu
Copy link
Contributor

jeffyu commented Jun 29, 2012

Hi Ivo,

Yeah, your point makes sense to me, I'll just not ignore the error thrown by undeployment procedure though.

Thanks for the Patch and Apply link, didn't went that route yet. :-)

Sorry for the late response, was kept distracted by other things these two days.

Regards
Jeff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants