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

DOL module vs. technical debt: generics #24082

Merged
merged 72 commits into from
Sep 21, 2022

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Aug 15, 2022

This PR is rather a "feature branch merge" than single fix, because all changes were done with a target to reduce the extreme tech debt of the DOL modules (deployment). Before the merge I would like to run all available tests to ensure that nothing broke, including TCK.

@dmatej dmatej added this to the 7.0.0 milestone Aug 15, 2022
@dmatej dmatej requested a review from arjantijms August 15, 2022 20:53
@dmatej dmatej self-assigned this Aug 15, 2022
@dmatej dmatej linked an issue Aug 15, 2022 that may be closed by this pull request
@dmatej
Copy link
Contributor Author

dmatej commented Aug 22, 2022

Note for myself - ejb_group_embedded passed locally twice.

@dmatej dmatej force-pushed the dol-refactoring-generics branch 4 times, most recently from 7a42966 to 08f0e6e Compare August 29, 2022 16:14
@dmatej
Copy link
Contributor Author

dmatej commented Sep 2, 2022

I will pause now, or slow down ... I will create another parallel PR for the TCK runner ... so I can ensure that this huge PR will not break anything. I committed part by part last week, because I could not find mistake I did ... now I found it. When I replaced "...Interceptors" string by Interceptor.class.getName() ... yeah, that missing s was it :-D

Bad was that I could not find it so long, good is that it forced me to analyze the API (broken, leaking) deeply and add some notes how to clean it up so it would make sense. After finishing this PR DOL will not be a scary monster any more :-) - despite it doesn't look like that, the worst part is done. The more code changes will come yet, but they should be much safer.

But again, TCK first.

- I tried to narrow generics in this packages, which made visible some weird
  constructions, maybe from some unfinished former refactoring.
- sometimes I inverted some conditions to make them more readable too, etc.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- unused and in PersistenceDescriptor was using heterogenous collection
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- recently we fixed typo in one name, so I created this.
- the package; probably we should create some JDK adapter where we would
  collect JDK enhancements.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- fixed copy and paste, equals instead of == for string
- reduced noise in logs - don't log twice, better set the exception cause.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- I forgot that I am in a cycle

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- the interface wasn't an interface, just a container of constants.
- all constants were used just on a single place

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- it did not use any instance's methods nor attributes
- writeDescriptor must accept just T as it is designed

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- amended commit where I originally renamed these methods, but I put
  the original name back, so I am committing just the cleanup around here.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- they dont use anything of the object

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- didn't use anything from the object

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
…wn object

- original code was confusing to read, caused inconsistencies when I improved
  generics, and led to unwanted usages. Also initiated maps which later
  weren't used at all.
- applied some conventions - removed copypasted javadoc of parent, public
  static methods first, don't set variables which you don't use any more...

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- AuthContext renamed to AuthContextImpl
- ClientAuthContext and ServerAuthContext have the new common parent,
  the AuthContext interface. More should be done later in own PR,
  now I just needed to know classes I am working with.
- this module is really ugly, should be radically refactored or removed.
- years ago I had to implement login module and realm - be careful when
  refactoring, protected fields are used by other implementations, so the
  refactoring will be a breaking change. This one shouldn't.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- private fields
- bit optimized logging + stacktrace instead of toString
- removed unused configSupport

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
…wn object

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- fixes mistake from recent commit

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej dmatej force-pushed the dol-refactoring-generics branch from b519bec to de13ab5 Compare September 18, 2022 17:18
@arjantijms arjantijms requested a review from russgold September 19, 2022 12:39
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- equals is not absolutely correct in the meaning of it's definitions,
  the desired mechanism is not equality, but if the method overrides
  another method. The order of processing of methods in the inheritance tree
  is given, that is why this work. After my refactoring it stopped working,
  because I tried to correctly implement equals (but it still wasn't symmetric).
- it should be probably replaced by different mechanism.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej dmatej marked this pull request as ready for review September 21, 2022 19:38
@dmatej
Copy link
Contributor Author

dmatej commented Sep 21, 2022

I'm yet passing TCK tests which failed yesterday, but it seems this should be the last fix. Refactoring will continue, but it would be good to have some safe point not just in the dev branch ...

04:02:19.443     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/lite/tx/cm/singleton/annotated/Client.java#localRequiresNewTest_from_ejbembed
04:02:19.443     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/lite/tx/cm/singleton/annotated/Client.java#localRequiresNewTest_from_ejblitejsp
04:02:19.443     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/lite/tx/cm/singleton/annotated/Client.java#localRequiresNewTest_from_ejbliteservlet
04:02:19.443     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/lite/tx/cm/singleton/annotated/Client.java#localRequiresNewTest_from_ejbliteservlet2
04:02:19.443     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/lite/tx/cm/singleton/annotated/JsfClient.java#localRequiresNewTest_from_ejblitejsf

ejb30/lite/tx/cm/singleton/annotated passed locally now.

06:21:08.172     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/bb/session/stateful/cm/allowed/Client.java#afterCompletionTest
06:21:08.172     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/bb/session/stateless/cm/allowed/Client.java#postInvokeTest
06:21:08.172     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/bb/session/stateless/cm/allowed/Client.java#preInvokeTest

ejb30/bb/session/stateful/cm/allowed passed locally now.
ejb30/bb/session/stateless/cm/allowed passed locally now.

04:40:17.148     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/timer/interceptor/aroundtimeout/singleton/annotated/Client.java#aroundTimeoutMethod_from_ejbliteservlet
04:40:17.148     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/timer/interceptor/aroundtimeout/singleton/annotated/Client.java#aroundTimeoutMethod2_from_ejbliteservlet
04:40:17.148     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/timer/interceptor/aroundtimeout/singleton/annotated/Client.java#invocationContextMethods_from_ejbliteservlet
04:40:17.148     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/timer/schedule/tz/Client.java#allTZ_from_ejbliteservlet
04:40:17.148     [runcts] OUT => [javatest.batch] FAILED........com/sun/ts/tests/ejb30/timer/schedule/tz/Client.java#shanghaiAndArgentinaTZ_from_ejbliteservlet

ejb30/timer/interceptor/aroundtimeout/singleton/annotated passed locally now
Those two TZ tests locally failed, but that might be caused by local TZ on my laptop.

Failures: 
02:26:03.528  [ERROR]   JAXRSClientIT.deleteWithGenericTypeStringTest:135->JAXRSClientIT.deleteWithGenericTypeStringTest:236->JAXRSClientIT.checkFutureString:1915 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.getWithResponseClassTest:187->JAXRSClientIT.getWithResponseClassTest:370->JAXRSClientIT.checkFutureOkResponse:1909 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.getWithStringClassTest:175->JAXRSClientIT.getWithStringClassTest:356->JAXRSClientIT.checkFutureString:1915 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.optionsWithStringClassTest:255->JAXRSClientIT.optionsWithStringClassTest:1095->JAXRSClientIT.checkFutureString:1915 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.postTest:307->JAXRSClientIT.postTest:1264->JAXRSClientIT.checkFutureOkResponse:1909 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.putTest:371->JAXRSClientIT.putTest:1476->JAXRSClientIT.checkFutureOkResponse:1909 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.putWithGenericTypeResponseTest:419->JAXRSClientIT.putWithGenericTypeResponseTest:1611->JAXRSClientIT.checkFutureOkResponse:1909 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.traceTest:435->JAXRSClientIT.traceTest:1686->JAXRSClientIT.checkFutureOkResponse:1909 Future cannot be done, yet! ==> expected: <true> but was: <false>
02:26:03.529  [ERROR]   JAXRSClientIT.traceWithGenericTypeStringTest:471->JAXRSClientIT.traceWithGenericTypeStringTest:1796->JAXRSClientIT.checkFutureString:1915 Future cannot be done, yet! ==> expected: <true> but was: <false>

REST passed locally twice, probably some issue with the Jenkins machine (cpu overload? 18 parallel docker machines)

@arjantijms arjantijms merged commit 2eac1f2 into eclipse-ee4j:master Sep 21, 2022
@dmatej dmatej deleted the dol-refactoring-generics branch September 22, 2022 07:35
@dmatej dmatej mentioned this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need more descriptive error message in case of an invalid schema
2 participants