[#4765] fix node name to be harmonized with xa node name #4806
Conversation
|
@tomjenkinson would you be so kind and check this PR? |
ochaloup
force-pushed
the
issue4765-harmonise-node-name-xa-node-name
branch
2 times, most recently
from
c56e421 to
e51cd28
Compare
| @@ -23,7 +25,8 @@ public void setNodeName(final TransactionManagerConfiguration transactions) { | |||
|
|
|||
| try { | |||
| arjPropertyManager.getCoreEnvironmentBean().setNodeIdentifier(transactions.nodeName); | |||
| TxControl.setXANodeName(transactions.xaNodeName.orElse(transactions.nodeName)); | |||
| jtaPropertyManager.getJTAEnvironmentBean().setXaRecoveryNodes(Collections.singletonList(transactions.nodeName)); | |||
| TxControl.setXANodeName(transactions.nodeName); | |||
There was a problem hiding this comment.
Do we genuinely need the TxControl setting here? I would imagine it can be controlled through the arjPropertyManager directly?
There was a problem hiding this comment.
this is the problematic issue of the whole narayana configuration. Once the TxControl class is first initialized (and that's decision of the JVM, not possible to be influence. normally the TxControl is initialized as class when accessed first time) then any change done in the arjPropertyManager has no effect for the further use.
As we can't be sure what setter is started first or where the TxControl data is needed for the first time during Quarkus startup I consider this as the safe way (or maybe even necessary).
A bit related point is that I created a jira about rethinking the configuration of the whole Narayana. That came from discussion with @mmusgrov, here https://issues.jboss.org/browse/JBTM-3215
There was a problem hiding this comment.
What we do have to be certain of is there are no transactions that could be created before this is called and indeed the recovery manager has not started running before it. Are you sure that is the case?
There was a problem hiding this comment.
No, I'm not sure if it's the case.
There was a problem hiding this comment.
I assume that this code must run before the transaction service is created? In which case I would expect no transactions could be created before this runs? Or is that not the case in Quarkus? IOW could there be a transaction created with the default (non-unique) node ID and that could therefore lead to data integrity issue?
/cc @mmusgrov
There was a problem hiding this comment.
@tomjenkinson the need of use TxControl.setXANodeName is not caused by time when the transaction service is created. The need is caused by the fact that we can't be sure that nobody else calls the TxControl setter. When it happens then all data are initialized. When this method is called after the setter was used before then changes in the bean have no effect.
In summary, by me the TxControl call is needed here.
There was a problem hiding this comment.
Thanks for confirming. I suppose you are less concerned about the setter being called (which as you say it is public) but rather when the class is loaded it will read the value as set in the arjPropertyManager: https://github.com/jbosstm/narayana/blob/master/ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/TxControl.java#L238
In that case, I understand the point - thanks!
| @@ -46,6 +49,7 @@ public void setDefaultProperties(Properties properties) { | |||
|
|
|||
| public void setDefaultTimeout(TransactionManagerConfiguration transactions) { | |||
| transactions.defaultTransactionTimeout.ifPresent(defaultTimeout -> { | |||
| arjPropertyManager.getCoordinatorEnvironmentBean().setDefaultTimeout((int) defaultTimeout.getSeconds()); | |||
There was a problem hiding this comment.
+1 - could the TxControl statement immediately following be removed too do you think?
There was a problem hiding this comment.
No, I assume it can't be. Just pointing that for WFLY we need to call TxControl. But WFLY configuration is more dynamic, so there it could be truly obvious.
ochaloup
force-pushed
the
issue4765-harmonise-node-name-xa-node-name
branch
from
e51cd28 to
2418c29
Compare
fixes #4765
Narayana node name needs to be correctly set and it should be documented what it's for in the Quarkus documentation.