snowdrop / narayana-spring-boot

Narayana Spring Boot autoconfiguration and starter
Apache License 2.0
46 stars 43 forks source link

Compute XA recovery nodes from node identifier by default #143

Closed graben closed 4 months ago

graben commented 5 months ago

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.

graben commented 5 months ago

Hi @geoand,

there are quite a lot of PRs open for long time. Could you please have a look at them if possible?

Thanks Benjamin

geoand commented 5 months ago

Hi,

Unfortunately I know very little about Narayana, so I am not the best person to review those

graben commented 5 months ago

Maybe I should ask @mmusgrov for his opinion to this specific task? But later on I need someone with commit rights ;)

geoand commented 5 months ago

Sounds reasonable :)

mmusgrov commented 4 months ago

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.

mmusgrov commented 4 months ago

there are quite a lot of PRs open for long time. Could you please have a look at them if possible?

@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.

mmusgrov commented 4 months ago

there are quite a lot of PRs open for long time. Could you please have a look at them if possible?

@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?

graben commented 4 months ago

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?

mmusgrov commented 4 months ago

They should already be kept in sync: TxControl takes the value from the config bean.

mmusgrov commented 4 months ago

@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:

With this change it is no longer possible to disable "bottom up" resource recovery. Although that is a good thing in most use cases, it does mean it cannot be disabled. Note that bottom up recovery consumes connection resources and if it were to be disabled the system admin/management would need to factor in any potential issues with resource locks for in-doubt branches. Note that WildFly takes the automatic recovery route too, like this PR does, but then again WildFly is an app server.

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.

geoand commented 4 months ago

Sure thing!

@jacobdotcosta could you kick off a release please?

mmusgrov commented 4 months ago

@geoand @jacobdotcosta What about the other open PRs that are upgrading dependencies, @graben needs those too.

geoand commented 4 months ago

Merged :)

geoand commented 4 months ago

@jacobdotcosta if you can do a release (3.2.0), that would be great

graben commented 4 months ago

Please hold release until update to Camel 4.4.3 is done (under vote since Sunday evening)

geoand commented 4 months ago

Understood, thanks