Skip to content

fix(recovery): Change initialiseConnection() in XAResourceRecoveryHelper #34

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

danila-m
Copy link

  • At least on Oracle & Postgres, the connections wrapped inside the XAConnections will automatically timeout after some time.
  • When it happens, the recovery mechanism does not detect that the XAConnections need to be closed & recreated (as the objects are not null). This will generate warning logs at regular intervals as the recovery mechanism tries to do its thing on "dead" connections.

This commit improves this behavior by:

  • Attempting to properly close the "active" connection&
  • then always recreating a connection during the recovery process.

- At least on Oracle & Postgres, the connections wrapped inside the XAConnections will automatically timeout after some time.
- When it happens, the recovery mechanism does not detect that the XAConnections need to be closed & recreated (as the objects are not null). This will generate warning logs at regular intervals as the recovery mechanism tries to do its thing on "dead" connections.

This commit improves this behavior by:
- Attempting to properly close the "active" connection&
- then always recreating a connection during the recovery process.
@KarmBot
Copy link
Collaborator

KarmBot commented Oct 27, 2021

Can one of the admins verify this patch? If it is not dangerous to our systems, juts tell me to "run_tests". ( without the underscore and quotes 😃 )

mmusgrov
mmusgrov previously approved these changes Oct 29, 2021
} catch (Exception e) {
log.debug("XAResourceRecoveryHelper : The connection could not close properly, generating a new one", e);
} finally {
connection = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I am reading this correctly then every call to getXAResources() will return a fresh connection. I'm not sure who calls it, but I am assuming it is called by the narayana recovery module, which means that each recovery pass will get a fresh resource. I am unclear about the performance impact but safety is more important than performance. On this basis I will confer my approval.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think you are right.

@zhfeng
Copy link
Contributor

zhfeng commented Oct 29, 2021

run_tests

@Karm
Copy link
Contributor

Karm commented Oct 29, 2021

run tests

1 similar comment
@Karm
Copy link
Contributor

Karm commented Nov 1, 2021

run tests

@Karm
Copy link
Contributor

Karm commented Nov 1, 2021

@danila-m CI failed on

[INFO] Starting audit...
[ERROR] /home/tester/jenkins/workspace/narayana-tomcat/f977fc1d/tomcat-jta/src/main/java/org/jboss/narayana/tomcat/jta/internal/PoolingDataSourceFactory.java:23: Using the '.*' form of import should be avoided - javax.sql.*. [AvoidStarImport]
Audit done.

@Karm
Copy link
Contributor

Karm commented Nov 1, 2021

The CI job needed reconfiguration and it should be fine now.

@zhfeng , if you comment run_tests without the underscore it will run again....

@danila-m
Copy link
Author

danila-m commented Nov 2, 2021

@danila-m CI failed on

[INFO] Starting audit...
[ERROR] /home/tester/jenkins/workspace/narayana-tomcat/f977fc1d/tomcat-jta/src/main/java/org/jboss/narayana/tomcat/jta/internal/PoolingDataSourceFactory.java:23: Using the '.*' form of import should be avoided - javax.sql.*. [AvoidStarImport]
Audit done.

Done

@Karm
Copy link
Contributor

Karm commented Nov 2, 2021

run tests

@Karm
Copy link
Contributor

Karm commented Nov 2, 2021

@mmadzin @jfclere Could you take it from here, please?
h2 tests passing, but those with real DBs don't, there are obsolete http instead of https URLs used etc.

Unable to collect/resolve dependency tree for a resolution due to: 
Failed to collect dependencies at org.postgresql:postgresql:jar:42.2.2, 
caused by: Server returned HTTP response code: 501 for 
URL: http://repo1.maven.org/maven2/org/postgresql/postgresql/42.2.2/postgresql-42.2.2.pom

(not related to this PR)

The project needs some attention :-)

@mmusgrov
Copy link
Collaborator

mmusgrov commented Feb 4, 2022

Hello @Karm I still approve of this change. Will you either merge it or withdraw the PR.

Unable to collect/resolve dependency tree for a resolution due to:
Failed to collect dependencies at org.postgresql:postgresql:jar:42.2.2,
caused by: Server returned HTTP response code: 501 for
URL: http://repo1.maven.org/maven2/org/postgresql/postgresql/42.2.2/postgresql-42.2.2.pom


(not related to this PR)

Is there an issue for this problem?

@Karm
Copy link
Contributor

Karm commented Feb 15, 2022

Hello @mmusgrov,

See #33

I haven't been involved with the project for years now.

@mmadzin, @csutherl or @jfclere could do the merging as they still own the project.

In addition to that, you yourself and @tomjenkinson are both committers on this repo with the power to merge changes.

Cheers
K.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants