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

Don't merge - Revert "Improve Narayana recovery manager service and integrate it with agroal" #28003

Closed

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Sep 16, 2022

This is a revert of #26161 .

Please don't merge for now. We have to discuss this with @mmusgrov @zhfeng and @Sanne .

See my comment here: #26161 (comment) .

I created this PR to make sure this stays on my radar and I don't release 2.13.0.Final without this situation fixed.

@gsmet gsmet added this to the 2.13.0.Final milestone Sep 16, 2022
@quarkus-bot quarkus-bot bot added area/agroal area/narayana Transactions / Narayana labels Sep 16, 2022
@gsmet gsmet changed the title WIP - Revert "Improve Narayana recovery manager service and integrate it with agroal" Don't merge - Revert "Improve Narayana recovery manager service and integrate it with agroal" Sep 16, 2022
@gsmet gsmet force-pushed the revert-narayana-recovery-service-2.13 branch from 783c239 to b5ebff1 Compare September 16, 2022 12:44
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 16, 2022

Failing Jobs - Building b5ebff1

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 18
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/oidc-code-flow 

📦 integration-tests/oidc-code-flow

io.quarkus.it.keycloak.CodeFlowTest.testTokenAutoRefresh line 497 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/tenant-autorefresh
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

@mmusgrov
Copy link
Contributor

I'm not sure that adding a property to disable recovery is wise. We've had this kind of request before: users want transactions but don't want the overhead of recovery, however JTA without recovery does not provide ACID guarantees so we've always resisted such requests.

I think what you really need is lazy initialisation of the recovery system when a transaction involving multiple participants starts or a property that says initialise recovery at start up. But both of these options require changes to narayana.

@zhfeng
Copy link
Contributor

zhfeng commented Sep 20, 2022

agroal needs to register AgroalXAResourceRecovery to XAResourceRecoveryRegistry when gernerating the DataSource at start up. So I have to say there is no need to lazy initialisation of the recovery system. Actually the issue is that RecoveryManagerService creates a directory ObjectStore at start up that is not wanted on customer side. But I think we will find a way to resolve it, see #28014 (comment)

@zhfeng
Copy link
Contributor

zhfeng commented Sep 20, 2022

And also if quarkus.datasource.jdbc.transaction=XA, we have to enable the recovery in narayana-jta implicty. That could be in 2.14, see #28025 (review)

@gsmet gsmet closed this Sep 20, 2022
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Sep 20, 2022
@gsmet
Copy link
Member Author

gsmet commented Sep 20, 2022

See #28025 for a better outcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agroal area/narayana Transactions / Narayana triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants