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

JBTM-1364 and WFLY-1808 #19

Merged
merged 2 commits into from Aug 14, 2013

Conversation

@gytis
Copy link
Member

commented Aug 1, 2013

No description provided.

@ghost ghost assigned paulrobinson Aug 1, 2013
@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 1, 2013

Started testing this pull request: http://172.17.131.2/job/btny-pulls-wildfly/163/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 1, 2013

Tests failed (http://172.17.131.2/job/btny-pulls-wildfly/163/): AS build failed

@gytis

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2013

Failed because of missing restat-bridge artefact. I guess we'll have to merge either this one or narayana pull request with the bridge without testing...


}

private boolean isBridgeRequired(final DeploymentUnit deploymentUnit) {

This comment has been minimized.

Copy link
@paulrobinson

paulrobinson Aug 2, 2013

Doesn't this return 'true' if either of the Transaction annotations are used anywhere in the deployment? Arn't we just interested in if they appear on the current endpoint we are deploying?

This comment has been minimized.

Copy link
@gytis

gytis Aug 2, 2013

Author Member

It does, I thought that was the decision? Because there would be the problem if some bean which is used by the service is annotated but the service isn't. Or should we assume that the service writer is responsible for annotating JAX-RS resources as well?

This comment has been minimized.

Copy link
@gytis

gytis Aug 2, 2013

Author Member

Also in some cases it may be quite difficult to find resources annotated with transactional annotations. For example, if the method in the parent class is annotated with @transactional while all the methods annotated with JAX-RS annotations are in subclasses, or vice versa.

This comment has been minimized.

Copy link
@paulrobinson

paulrobinson Aug 2, 2013

I think it's better to require the developer to add transactional annotations to their JAX-RS endpoints. The alternative, is that we add the bridge handler to all JAX-RS endpoints even if the transaction annotations are only used in a single, unrelated, place. We can add a new annotation in the future to explicitly enable bridging for those applications that can't put the Transaction annotations directly on the JAX-RS endpoints.

Can you see if this is feasible?

This comment has been minimized.

Copy link
@paulrobinson

paulrobinson Aug 5, 2013

Does your fix work if @transactional is placed on a method, or does it have to be on the class?

This comment has been minimized.

Copy link
@gytis

gytis Aug 5, 2013

Author Member

It does work. classInfo.annotations() returns list of all annotations defined in the class (both type and method). See this test resource: https://github.com/Gytis/jboss-as/blob/e58b229417d674f6ae77e73ce3461ea0c0b327bf/testsuite/integration/rts/src/test/java/org/wildfly/test/extension/rts/common/InboundBridgeResource.java

This comment has been minimized.

Copy link
@paulrobinson

paulrobinson Aug 5, 2013

Great, looks good!

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 2, 2013

Started testing this pull request: http://172.17.131.2/job/btny-pulls-wildfly/164/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 2, 2013

Started testing this pull request: http://172.17.131.2/job/btny-pulls-wildfly/165/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 2, 2013

Tests failed (http://172.17.131.2/job/btny-pulls-wildfly/164/): AS build failed

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 2, 2013

Tests failed (http://172.17.131.2/job/btny-pulls-wildfly/165/): AS build failed

@gytis gytis referenced this pull request Aug 12, 2013
@gytis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2013

Quick update on failures. These two tests fail only when executed with all RTS tests. They succeed once executed separately.
Since these tests fail to start a new REST-AT transaction and they don't use bridge directly, the problem may be in not clearing something after previous test.

@gytis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2013

Apparently basic action fails to be started because it is marked rollback only. Taking InboundBridgeTest out allows another tests to pass...

@gytis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2013

Adding Assert.assertEquals(0, txSupport.txCount()); at the end of @after method solved the problem. Will update both PRs with it.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 13, 2013

Started testing this pull request: http://172.17.131.2/job/btny-pulls-wildfly/166/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 13, 2013

Tests failed (http://172.17.131.2/job/btny-pulls-wildfly/166/): AS build failed

@paulrobinson

This comment has been minimized.

Copy link

commented Aug 13, 2013

I think you also need to rebase as we have moved onto WildFly Beta1.

@gytis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2013

5_BRANCH is still on Alpha4-SNAPSHOT

@paulrobinson

This comment has been minimized.

Copy link

commented Aug 13, 2013

It was Blacktie that hadn't been updated. That's done now.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 13, 2013

Started testing this pull request: http://172.17.131.2/job/btny-pulls-wildfly/167/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Aug 13, 2013

Tests failed (http://172.17.131.2/job/btny-pulls-wildfly/167/): AS build failed

@paulrobinson paulrobinson merged commit 04d6da9 into jbosstm:5_BRANCH Aug 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.