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

Improve transactional observer without jta warning #14968

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

Postremus
Copy link
Member

@Postremus Postremus commented Feb 10, 2021

closes #14981

@ghost ghost added the area/arc Issue related to ARC (dependency injection) label Feb 10, 2021
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should also have a tracking issue.

@Postremus Postremus changed the title Remove transactional observer without jta warning Improve transactional observer without jta warning Feb 10, 2021
The warning now includes, that the observers are still triggered, which is a clearer messaging.
@Postremus
Copy link
Member Author

Failure in Integration Tests - Container Image - Invoker FAILURE [02:42 min] do not seem related.

@@ -99,8 +99,9 @@ static ObserverInfo create(String id, BeanDeployment beanDeployment, DotName bea
} else {
info = beanClass.toString();
}
LOGGER.warnf("The observer %s makes use of %s transactional observers but no " +
"JTA capabilities were detected.", info, transactionPhase);
LOGGER.warnf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the chat I understood we wanted to only show this when !jtaCapabilities and avoid it entirely in case of no tx in progress? E.g. IMO the condition on L#94 should only check for JTA cappabilities.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0.html#transactional_observer_methods

A transactional observer method may be declared by specifying any value other than IN_PROGRESS for during

Removing the check for !transactionPhase.equals(TransactionPhase.IN_PROGRESS) would trigger this warning for normal observers as well when no jta is present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I wasn't clear - I meant checking if the method has any transaction phase and there are no JTA capabilities.

However, looking into code it looks like we are automatically giving all OMs a default phase (see this code). @mkouba do you happen to know why is that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use TransactionPhase.IN_PROGRESS if no explicit Observes.during() is set, i.e. we use the default value from the annotation. Note that Jandex does not return the default value unless AnnotationInstance.valueWithDefault() is used (because defaults are stored on the annotation's defining class).

Base automatically changed from master to main March 12, 2021 15:55
@gsmet
Copy link
Member

gsmet commented Jul 13, 2021

@manovotn @mkouba could we drive this puppy home or close it? Thanks!

@mkouba mkouba merged commit 8f9d0df into quarkusio:main Jul 13, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve transactional observer without jta warning
4 participants