PAYARA-4197 Minor but large cleanup in ejb-container and related things #4325

dmatej · 2019-11-15T06:11:23Z

sorted imports, removed unused
code formatting
- preferred shorter block first
- preferred positive if without negation
- preferred fast return
- collisions of these rules resolved appropriately
- removed 10 years old TODOs
using generics where it is clear
trivial optimizations
- declaring variables
- preferred final instead of changing the default value
removed redundant keywords (public method in interface, ie)
removed redundant class typing
using addSuppressed/cause rather than masking exceptions
improved logging
- log entry to important method at least as finest
- removed some duplicit logs

Description

This is an enhancement for better readibility and also to provide better informations in logs.
EJB timeout warnings are now printed immediately when the transaction is set to the rollback only state, and later are printed another informations about the rollback - why it is done. These messages are not duplicated, but together lead the developer to some idead what happened and why.

Important Info

Dependant PRs

Blockers

This PR continues the work started in #4323 in much larger scale

Testing

New tests

Experimental test will be pushed in #4263

Testing Performed

Test suites executed

Quicklook - OK
Payara Samples - not executed
Java EE7 Samples - OK
Java EE8 Samples - OK
Payara Private Tests - not executed
Payara Microprofile TCKs Runner - not executed
Jakarta TCKs - ejb32 - fails one test at this moment, but it is not related.

Testing Environment

Manual tests: Kubuntu 19.10, OpenJDK11
TestContainers: Debian 11, OpenJDK11
TCK: JDK8

Notes for Reviewers

The code needs much more love :-)

- sorted imports, removed unused - code formatting - preferred shorter block first - preferred positive if without negation - preferred fast return - collisions of these rules resolved appropriately - removed 10 years old TODOs - using generics where it is clear - trivial optimizations - declaring variables - preferred final instead of changing the default value - removed redundant keywords (public method in interface, ie) - removed redundant class typing - using addSuppressed/cause rather than masking exceptions - improved logging - log entry to important method at least as finest - removed some duplicit logs

...ectors/work-management/src/main/java/com/sun/enterprise/connectors/work/WorkCoordinator.java

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/EjbInvocation.java

...erver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSessionContextImpl.java

...erver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java

dmatej · 2019-11-15T07:00:20Z

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/BaseContainer.java

@@ -465,7 +457,7 @@
    // an EJB Remote interface to be a subtype of the Service Endpoint
    // Interface.  In that case, it's ambiguous to do a lookup based only
    // on a java.lang.reflect.Method
-    protected Map webServiceInvocationInfoMap = new HashMap();
+    protected Map<Method, InvocationInfo> webServiceInvocationInfoMap = new HashMap<>();


Now I see, it should be final. But it is protected, so Eclipse was afraid that it could break something. It could, but not in Payara.

I'd be in favour of making it final. Also I wonder if those map should not be concurrent ones. BaseContainer sounds like multiple threads to me.

True, there is too much synchronization (or none here). But I'm afraid of that, TCK is sequential and slow, but I'm not sure with applications with thousands of threads. The code is not much well prepared for concurrency and we don't have much tests for that.
I will try to create some later ...

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/BaseContainer.java

...r/ejb/ejb-container/src/main/java/com/sun/ejb/containers/EJBContainerTransactionManager.java

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/SessionContextImpl.java

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/StatefulSessionContainer.java

...rver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/WebServiceInvocationHandler.java

appserver/ejb/ejb-full-container/src/main/java/org/glassfish/ejb/mdb/MessageBeanContainer.java

appserver/ejb/ejb-internal-api/src/main/java/org/glassfish/ejb/api/EJBInvocation.java

...rver/transaction/jta/src/main/java/com/sun/enterprise/transaction/JavaEETransactionImpl.java

dmatej · 2019-11-15T08:11:10Z

...rver/transaction/jta/src/main/java/com/sun/enterprise/transaction/JavaEETransactionImpl.java

    public void setRollbackOnly() throws IllegalStateException, SystemException {
        checkTransationActive();
-        if ( jtsTx != null )


Say no to if/else in negation!

It all depends. Readability is not about following a set of rules. Its a property of the whole. There might be cases where a negation reads better and nobody should start asking to apply some rule. Here on the other hand flipping makes it more readable :)

This is not the case. Maybe I should use smileys :)
Usually negation is maybe most usual cause of misunderstandings. Especially when I am tired and it is used in a sequence of ternary operators ...

jbee

That was a big PR. Please if you do bigger cleanups split them into pure "cosmetic" changes which don't require a deeper reasoning on all changes and those that do semantic changes that a reviewer checks if they are the right thing to do in the particular situation. Especially full class formatting and other fully automated cosmetic changes should be kept separate.

You did change logging to use lambdas to avoid "doing work" with string concatenation. These usually did have a closure on a local variable or method parameter which means the lambda will be created for each invocation. Not sure this is saving us anything. So if we want to avoid doing unnecessary work we either need a if-guard or use a log method where pattern and arguments are passed separately.
I stopped marking all occurrences at some point and this isn't terribly important but it maybe should be consistent in style however you want to have this done.

When it comes to if-return sequences I prefer to not have them use else. Somehow this makes it look more complicated than it is. I stopped marking those at some point.

I did also mark a couple of HashMap usages that I suspected should really be ConcurrentHashMaps. Not your change but still something we should maybe look at.

So overall there isn't one thing you really need to change but I made a lot of comments so I imagine there will be a number of changes.

jbee · 2019-11-15T07:43:50Z

...ectors/work-management/src/main/java/com/sun/enterprise/connectors/work/WorkCoordinator.java

@@ -412,6 +419,7 @@ public String toString() {
     * @param state CREATED or Either STARTED or COMPLETED or TIMEDOUT
     */
    public synchronized void setState(int state) {
+        logger.finest(() -> String.format("setState(state=%s)", state));


This lambda might save a unneeded format but costs creating a fresh lambda instance son each call of setState since it captures the state variable.
If we truly want to make this less expensive we have to do the ugly if level check around it.

I would defend suppliers, because the code is optimized by JVM and I believe it is still faster than if block with JUL formatter; especially when you enable finest.
But we can argue about nanoseconds, the JUL logging is extremely slow by default because of poor implementation of handlers and formatters, concatenating strings, etc.
If I remember well, the lambda here is created pretty fast, because it is not generated in runtime b some other method, it uses some template and even JIT can inline it.

But for sure, I will create some test for that, because I don't believe too much what I wrote :)

See test results I posted somewhere in the begining. It surprised even me - you can safely use lambdas where posiible, I'm pretty sure that if you don't use a method to generate new lambda, it is still the same.

jbee · 2019-11-15T07:46:27Z

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/EjbInvocation.java

-{
-
+public class EjbInvocation //
+    extends ComponentInvocation //


I am happy with using these guides for a automatic formatter but we never discussed something like it in the team as far as I am aware. Maybe it is time to talk about this before use it extensively.

In fact I don't know any other option to have stable formatting. This only prevents manually formatted code before automatic reformatting which will someone do and would not notice that something breaks when he tries to fix something else ...

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/EjbInvocation.java

jbee · 2019-11-15T10:27:01Z

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/StatelessSessionContainer.java

@@ -111,43 +118,37 @@
    // Data members for Remote business view. Any objects representing the
    // Remote business interface are not subtypes of EJBObject.
    private EJBObjectImpl theRemoteBusinessObjectImpl = null;
-    private Map<String, java.rmi.Remote> theRemoteBusinessStubs =
-        new HashMap<String, java.rmi.Remote>();
+    private final Map<String, Remote> theRemoteBusinessStubs = new HashMap<>();


another non concurrent map where I'd expect something else. hm.

...tainer/src/main/java/org/glassfish/persistence/ejb/entitybean/container/EntityContainer.java

jbee · 2019-11-15T10:42:36Z

...rver/transaction/jta/src/main/java/com/sun/enterprise/transaction/JavaEETransactionImpl.java

    public void setRollbackOnly() throws IllegalStateException, SystemException {
        checkTransationActive();
-        if ( jtsTx != null )


It all depends. Readability is not about following a set of rules. Its a property of the whole. There might be cases where a negation reads better and nobody should start asking to apply some rule. Here on the other hand flipping makes it more readable :)

...ion/jta/src/main/java/com/sun/enterprise/transaction/JavaEETransactionManagerSimplified.java

- reduced concats - sometimes reduced complexity

dmatej · 2019-11-16T02:36:24Z

Jenkins test please

dmatej · 2019-11-16T02:38:01Z

Good result:

jbee

Have lost track a bit on the things beyond your changes that we might want to check but I think you got this. Good work!

dmatej added the PR: DO NOT MERGE Don't merge PR until further notice label Nov 15, 2019

dmatej requested review from jbee and cubastanley November 15, 2019 06:11

dmatej self-assigned this Nov 15, 2019