-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Ensure the narayana-jta extension fully shuts down the recovery manager #35174
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,7 +16,6 @@ | |||||||||||||||||
import com.arjuna.ats.arjuna.common.arjPropertyManager; | ||||||||||||||||||
import com.arjuna.ats.arjuna.coordinator.TransactionReaper; | ||||||||||||||||||
import com.arjuna.ats.arjuna.coordinator.TxControl; | ||||||||||||||||||
import com.arjuna.ats.arjuna.recovery.RecoveryManager; | ||||||||||||||||||
import com.arjuna.ats.internal.arjuna.objectstore.jdbc.JDBCStore; | ||||||||||||||||||
import com.arjuna.ats.jta.common.JTAEnvironmentBean; | ||||||||||||||||||
import com.arjuna.ats.jta.common.jtaPropertyManager; | ||||||||||||||||||
|
@@ -158,7 +157,14 @@ public void startRecoveryService(final TransactionManagerConfiguration transacti | |||||||||||||||||
public void handleShutdown(ShutdownContext context, TransactionManagerConfiguration transactions) { | ||||||||||||||||||
context.addLastShutdownTask(() -> { | ||||||||||||||||||
if (transactions.enableRecovery) { | ||||||||||||||||||
RecoveryManager.manager().terminate(true); | ||||||||||||||||||
try { | ||||||||||||||||||
QuarkusRecoveryService.getInstance().stop(); | ||||||||||||||||||
} catch (Exception e) { | ||||||||||||||||||
// the recovery manager throws IllegalStateException if it has already been shutdown | ||||||||||||||||||
log.warn("The recovery manager has already been shutdown", e); | ||||||||||||||||||
Comment on lines
+162
to
+164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not catch exception if we expect something more specific.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yrodiere The signature of the method that throws the IllegalStateException is Note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I get what you mean. But you only know that Any other exception would be unexpected, so even if you have to catch them, you probably shouldn't log a warning saying the recovery manager has already been shut down... ? |
||||||||||||||||||
} finally { | ||||||||||||||||||
QuarkusRecoveryService.getInstance().destroy(); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
TransactionReaper.terminate(false); | ||||||||||||||||||
}); | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -51,4 +51,11 @@ public void create() { | |||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
xaResources.clear(); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||||||||||||||
public void destroy() { | ||||||||||||||||||||||||||||||||||||
super.destroy(); | ||||||||||||||||||||||||||||||||||||
isCreated = false; | ||||||||||||||||||||||||||||||||||||
recoveryManagerService = null; | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks good for the purposes of this specific issue, but shouldn't this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zhfeng will you respond to Sanne's point about lack of protection against concurrent access, thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I think the real working codes are in RecoveryManager.java. Both of initialize/terminate methods are protected by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, That being said, the superclass |
||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
Comment on lines
+55
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add a non-regression test for this? You can find an example of a hot reload test in Hibernate ORM tests:
In your case you'd probably just have a resource that runs an transaction and check that the resource still works after changing it (and automatically triggering a hot reload) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I doubt that I will have time today, but do note that the clock is ticking on this fix. If we can merge it now then I or Amos can come back to it later. I have a similar response to you next review comment as well. These extra comments are good but they were present in the original code, I have just supplied the urgent bug fix to the existing code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the comment about concurrency in particular is not urgent at all. As to whether we should add non-regression tests when fixing bugs, well, I'll let whoever wants the fix urgently decide. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually on second thoughts, regression tests ought to be non-negotiable so I will prioritise that ahead of other stuff today, thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the example you gave. what is it that triggers the hot reload? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And do I need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not, since you don't want to test devservices here. You'll probably want to set the JDBC URL though. But it looks like you use a test profile for that in other tests, not Lines 8 to 21 in fd02a43
Line 11 in 6fc8350
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've figured it out, I'll let you know if I hit any issues with it ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yrodiere @Sanne @gsmet apart from not having a regression test, I believe that I have resolved the main review comments. To make early progress, how about the following proposal: This bug fix gets merged today in its current state, I've fixed most of the comments. Then I will follow up with a new issue(s) to:
The follow up work will be done by either Amos, Marco or myself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this change for the reproducer described on the issue #34576 and this solves the issue. Stopping the QuarkusRecoveryService is definitely what we need here instead of the termination of the recovery manager only. +1
We probably haven't spotted it before because when starting the application anew we don't notice this problem, this is definitely something we should take into consideration for next tests.