Issue 24900 part 1: document and refactor, part 2: add first step in PoolManagerImpl unit test

[escay]

Draft for now, I would like to add more unit tests and I would like to remove a number of TODO statements I added in ConnectionPool and PoolManagerImpl classes.

Part 1 for issue #24900 Document and rename some ResourceHandle fields. Document and rename some ConnectionPool fields. Remove some unused methods and constructor parameters. Functional / possible incompatibility changes in DataStructureFactory and in ResourceHandle due to constructor changes.
(Maybe part 1 is a bit too much, I altered a few interfaces and removed unused parameters)

Part 2 for issue #24900 Start with a new PoolManagertImpl unit test to understand enlisted versus busy states of Resource handles and the wiring inside a transaction to keep track of all used resources.

[escay]

Part 3 for issue #24900 adds some more documentation. Main item of this commit is:

• Changed unit test to show 2 points where I would expect different behaviour. In case of resourceAbortOccurred and resourceErrorOccurred the connectionErrorOccurred flag of the resource is not set, while it is set in badConnectionClosed logic.
• Changed unit test to show removing resources from a transaction seems to behave as expected for basic scenarios.

[escay]

Part 4 for issue #24900 adds more detailed logging I used in the last months. During rebase I made some merge errors, so commit also contains some merge work from part 1,2,3.
Some import order is wrong, local fast build succeeded / did not fail on the new checkstyle rules for imports.

[dmatej]

I am bit confused why there's so many changes and some look like they already are in the master branch. Is it some GitHub issue? Seems so ... I tried locally:

• PoolManagerImplTest contains windows eolns, You can configure it in Eclipse IDE and other editors. Weird that Jenkins did not complain.... Aha, I did not add this rule to checkstyle.xml yet ...
• It looks well otherwise

[escay]

@dmatej Not sure what happened. I had a few tasks from May I wanted to rebase to the most recent master (maybe I used an incorrect approach into getting all master tasks into this branch), then I made a few mistakes in merging the import changes. I will try to clean it up and make a correct pull request. This should have been draft so far.

[dmatej]

Just replace those crlf with lf (linux) and I believe it will be alright. I tried to fetch and compare your brach with master and it seems ok, just github is somehow confused and shows diff to some old commit.

[escay]

Not sure why the last build failed and the build before the 'crlf' fix did not fail. Perhaps a rebuild is needed, but I cannot start it I think.
The commits together:

Part 1 for issue #24900 Document and rename some Resource…
b1d3069

Part 2 for issue #24900 Start with a new PoolManagertImpl…
6ec3390

Part 3 for issue #24900 More documentation and more unit …
bb41ac5

Part 4 for issue #24900 Add more detailed logging to impr…
027b7d2

Merge branch 'issue_24900' of https://github.com/escay/glassfish into…
f000acd

Fix checkstyle for issue #24900 merge.
d9d1f62

Change 'git ls-files --eol' value i/-text to i/lf
e902173

Solve issue #24900
The TODOs (mainly around PoolManagerImpl) I added in the code I plan to investigate later on to see if I can improve/solve them.

[pzygielo]

472 commits, 4006 files - seriously?

[escay]

472 commits, 4006 files - seriously?

I did a rebase last week to get my changes from May in this branch to work, it is only the last 7 commits. Maybe I rebased wrong as mentioned above. Github somehow shows all changes since June in the full list, which are not mine.

[pzygielo]

472 commits, 4006 files - seriously?

I did a rebase last week to get my changes from May in this branch to work, it is only the last 7 commits. Maybe I rebased wrong as mentioned above.

I will try to clean it up and make a correct pull request.

I was waiting for that.

[escay]

I didn't because @dmatej told me to just commit the lf change.
@pzygielo and @dmatej Let me know what to do.

[pzygielo]

Why changes from 8.0 are merged-in? I'm against pulling them in with this PR.

Edit: the commit I meant was already in master. 😞

[pzygielo]

I didn't because @dmatej told me to just commit the lf change.

But it didn't work, did it? And yet you marked as ready to review. So I comment on what I see.

[escay]

I think the crlf / lf linefeed change in 1 unit test file succeeded. I do not know why the build failed in the ejb_group_2 set, and I cannot start a new build to see if it was an environment issue. It was not the linefeed change in the unit test file.
I only rebased to master as far as I know, I do not know the 8.0 branch names.

If needed I can create a new branch and do everything fresh, so you can more easily review 4 cleaner commits.

[pzygielo]

I cannot start a new build to see if it was an environment issue.

Started build no. 11.

[pzygielo]

@escay Does this https://github.com/eclipse-ee4j/glassfish/compare/master...pzygielo:glassfish:pr24901?expand=1 look like the changes intended?

[escay]

@pzygielo It looks like the changes indeed. But I do not see a few new files like InjectionUtil and PoolManagerImplTest.

[pzygielo]

I do not see a few new files like InjectionUtil and PoolManagerImplTest.

True, I see I've missed untracked on commit.
Updated my branch for consistency.

[dmatej]

I reviewed using this command without --stat. GitHub's UI failed in this case.

git diff -w -M eclipse/master..escay/issue_24900 --stat

[OndroMih]

Is it maybe better to close this PR and create it again, so that it contains only the real changes?

[escay]

(No problem for me, just let me know)

[pzygielo]

just let me know

@escay - please do

[dmatej]

GOTO replacement of this PR: #25155