Compute XA recovery nodes from node identifier by default #143
Conversation
|
Hi @geoand, there are quite a lot of PRs open for long time. Could you please have a look at them if possible? Thanks |
|
Hi, Unfortunately I know very little about Narayana, so I am not the best person to review those |
|
Maybe I should ask @mmusgrov for his opinion to this specific task? But later on I need someone with commit rights ;) |
|
Sounds reasonable :) |
|
The node identifier should be unique (amongst instances that wish to share resource managers or objectstores) as explained in narayana config bean javadoc. Apart from that restriction you can use whatever you like. |
@graben we no longer get involved with the snowdrop project, for a number of years now, and I'm not sure who would be the best person to ask but I'd suggest that you take a look at the project commit history or the PR merge history. |
That said, your PRs updating various versions of dependencies all look good but can someone from the snowdrop team run some tests to verify that the new versions still work with snowdrop? @cmoulliard can you help Benjamin determine who to ping to get his PRs reviewed please? |
|
Hi @mmusgrov, thanks for your comments. I think the question you might be a good person to ask according to this PR is the change that node identifier and xa recovery nodes should be in sync by default. There are actually two different configuration points in snowdrop integration of Narayana that the user is in charge to keep them in sync. Can you confirm that the assumption is right that changing the default to a smart sync is correct? |
|
They should already be kept in sync: TxControl takes the value from the config bean. |
| @@ -128,7 +128,7 @@ public class NarayanaProperties { | |||
| /** | |||
| * XA recovery nodes. | |||
| */ | |||
| private List<String> xaRecoveryNodes = List.of("1"); | |||
| private List<String> xaRecoveryNodes = List.of(); | |||
There was a problem hiding this comment.
Nice, I agree that it should not default to "1".
| @@ -67,6 +67,9 @@ private void setNodeIdentifier(String nodeIdentifier) { | |||
| } | |||
|
|
|||
| private void setXARecoveryNodes(List<String> xaRecoveryNodes) { | |||
| if (xaRecoveryNodes.isEmpty()) { | |||
| xaRecoveryNodes = List.of(getPopulator(CoreEnvironmentBean.class).getNodeIdentifier()); | |||
There was a problem hiding this comment.
I think if the caller (ie the user via the properties) is asking for no "bottom up" recovery (xaRecoveryNodes tells narayana which orphaned XA resources it should try to recover) then this code should be respecting that choice. If you want it to be explicit you could log an info message here, but I'd just respect the config and not try to override it.
There was a problem hiding this comment.
If you do apply a property override then it would be a change in behaviour and users would need to be aware of that. Could you point me at the snowdrop documentation about this property.
There was a problem hiding this comment.
As far as I know the only doc is the readme.md. But there might exist more for the internal Redhat fork?
|
@geoand and @graben since there appear to be no docs, can we include a release note for this change (there is no associated issue anywhere for me to attach the release note). Something like: The reason I think it is worth mentioning to users is that I have seen questions asking how to disable recovery, for example I believe Quarkus offers the option. The last sentence about WildFly may not be appropriate for Spring Boot docs. |
|
Sure thing! @jacobdotcosta could you kick off a release please? |
|
@geoand @jacobdotcosta What about the other open PRs that are upgrading dependencies, @graben needs those too. |
|
Merged :) |
|
@jacobdotcosta if you can do a release ( |
|
Please hold release until update to Camel 4.4.3 is done (under vote since Sunday evening) |
|
Understood, thanks |
It's better to compute the XA recovery nodes from node identifier value, if not explicitly set. Avoids to set it twice if using custom node name.