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.

Sign up for GitHub

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

Jump to bottom

FISH-790 Refactor, fix bugs in JavaEE Context Utility #5010

Merged
merged 5 commits into from
Dec 3, 2020
Merged

FISH-790 Refactor, fix bugs in JavaEE Context Utility #5010

merged 5 commits into from
Dec 3, 2020

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Nov 27, 2020 •
edited
Loading

Description

Refactored Context utilities into immutable, serializable class.
Fixed many bugs along the way.

Important Info

Blockers

This PR is a blocker for Hazelcast 4 integration.

Testing

New tests

Added a unit test

Testing Performed

All test suites pass, tested with Hazelcast 4 branch

Testing Environment

Java 8, Mac

Documentation

Documentation added via Internal API javadoc for new EE context API

Notes for Reviewers

This issue includes APIs necessary for Hazelcast 4 to be much more tightly integrated with Jakarta EE and Payara
when you think invocation.preInvoke() this is easy-to-use replacement and so much more!
This refactoring has been in the works for 6+ months, so it's not "new" in any sense of the word

@lprimak lprimak marked this pull request as draft November 27, 2020 07:13
@lprimak
Copy link
Contributor Author

lprimak commented Nov 27, 2020

jenkins test

1 similar comment
@lprimak
Copy link
Contributor Author

lprimak commented Nov 27, 2020

jenkins test

@lprimak lprimak marked this pull request as ready for review November 27, 2020 21:30
@lprimak lprimak marked this pull request as draft November 28, 2020 01:43
@lprimak lprimak force-pushed the Refactor-CTX-Util branch 3 times, most recently from 65dc1ab to 333d8cc Compare November 28, 2020 05:00
@lprimak
Copy link
Contributor Author

lprimak commented Nov 28, 2020

jenkins test

@lprimak
Copy link
Contributor Author

lprimak commented Nov 28, 2020

jenkins test

@lprimak lprimak mentioned this pull request Nov 28, 2020
Closed
@lprimak lprimak changed the title FISH-790 Refactor JavaEE Context Util FISH-790 Refactor JavaEE Context Util, add context APIs to Payara Nov 29, 2020
@lprimak lprimak marked this pull request as ready for review November 29, 2020 02:25
@lprimak lprimak mentioned this pull request Nov 29, 2020
Merged
@lprimak
Copy link
Contributor Author

lprimak commented Nov 30, 2020

Fixes #5000

jbee
jbee reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

I did a first rough review. I feel I would want way more description of what this PR tries to do, what problems are behind it. As far as I could see this mostly adds a new API and its implementation but isn't used in so many places. Where it is used the main change appears to be to use an instance (something got bound I guess) instead of what corresponds to a generic context. Before I look closer I really would like to get better understanding so please add some paragraphs to the description of the PR that help with that. Thanks.

api/payara-api/src/main/java/fish/payara/context/ContextProducer.java Outdated Show resolved Hide resolved
...ver/web/gf-web-connector/src/main/java/fish/payara/appserver/context/ContextualizerImpl.java Show resolved Hide resolved
...b/weld-integration/src/main/java/org/glassfish/cdi/transaction/TransactionScopedCDIUtil.java
@@ -219,7 +219,7 @@ public BeanWrapper(Class<?> beanClass) {
catch(MultiException e) {
log(e.getMessage());
}
this.ctxUtil = ctxUtil;
this.ctxUtil = ctxUtil.map(JavaEEContextUtil::currentInvocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this deserves a explaining comment as it looks to me you try to address an issue here - was there one? which?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context Util API changed, this line just tracks that change, it would be a compile error otherwise.
It's self-explanatory for me here and shouldn't require a comment, as it would be like
"tracking API changes"

nucleus/common/internal-api/src/main/java/org/glassfish/internal/api/JavaEEContextUtil.java Show resolved Hide resolved
...zelcast-bootstrap/src/main/java/fish/payara/nucleus/hazelcast/PayaraHazelcastSerializer.java Outdated Show resolved Hide resolved
@lprimak
Copy link
Contributor Author

lprimak commented Nov 30, 2020

@jbee Please read the Javadoc in the new public API. This should explain most, if not all of your questinos

@lprimak lprimak requested a review from jbee November 30, 2020 14:48
@lprimak lprimak requested a review from pdudits November 30, 2020 14:48
@lprimak
Copy link
Contributor Author

lprimak commented Nov 30, 2020

jenkins test

@lprimak
Copy link
Contributor Author

lprimak commented Nov 30, 2020

@jbee @pdudits Since the public API is such a hot button issue and this PR is meant as a bug fix, I moved it into internal API so none of the above is an issue anymore

@lprimak
Copy link
Contributor Author

lprimak commented Nov 30, 2020

jenkins test

@lprimak lprimak changed the title FISH-790 Refactor JavaEE Context Util, add context APIs to Payara FISH-790 Refactor, fix bugs in JavaEE Context Utility Nov 30, 2020
@jbee
Copy link
Contributor

jbee commented Dec 3, 2020

@lprimak

Please read the Javadoc in the new public API. This should explain most, if not all of your questinos

Code tells me what is going on not why. I'd like to know you motivations both towards the problem and the chosen solution so I can understand this change in context.

jbee
jbee approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@jbee jbee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I dont't see any obvious issues with the PR but likewise it is hard to tell how or where it would introduce new issues. This is just an area that is hard to verify other than understanding the consequences of the change and its impact which is a hard nut to crack for this one. I'm quite convinced this is an improvement over the old code but ultimately I feel this is your call @MattGill98 being responsible for community.

@MattGill98 MattGill98 merged commit 07cc0af into payara:master Dec 3, 2020
@lprimak lprimak deleted the Refactor-CTX-Util branch December 3, 2020 18:04
@lprimak lprimak mentioned this pull request Dec 4, 2020
Merged
@Pandrex247 Pandrex247 mentioned this pull request Oct 4, 2021
Merged
aubi pushed a commit to aubi/Payara that referenced this pull request Jan 3, 2022
@MattGill98 @Pandrex247
FISH-790 Merge pull request payara#5010 from flowlogix/Refactor-CTX-Util
e58cbd6 
FISH-790 Refactor, fix bugs in  JavaEE Context Utility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdudits pdudits pdudits approved these changes

@jbee jbee jbee approved these changes

@Pandrex247 Pandrex247 Awaiting requested review from Pandrex247

@jGauravGupta jGauravGupta Awaiting requested review from jGauravGupta

@cubastanley cubastanley Awaiting requested review from cubastanley

@AlanRoth AlanRoth Awaiting requested review from AlanRoth

@MattGill98 MattGill98 Awaiting requested review from MattGill98

@Cousjava Cousjava Awaiting requested review from Cousjava

@dmatej dmatej Awaiting requested review from dmatej

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lprimak @jbee @pdudits @MattGill98