Lra client is started based on config instead of parameter value

[kikusko77]

NarayanaLRAClient client = new NarayanaLRAClient("http://localhost:49101/lra-coordinator/lra-coordinator");

When i am testing failure of the client, I discovered that if the coordinator pasted there isnt available, it will eventually start the one from lra.coodinator.urls.

private void clusterConfig(URI coordinatorUrl) {
if (CONFIG.getOptionalValue(COORDINATOR_URLS_KEY, String.class).isEmpty()) {
this.coordinatorUrl = coordinatorUrl;
this.coordinatorCount = 1;
} else {
String coordinators = getConfigProperty(COORDINATOR_URLS_KEY, coordinatorUrl.toASCIIString());

        this.coordinatorUrl = toURI(coordinators.split(",")[0]);
        this.coordinatorCount = coordinators.chars().filter(ch -> ch == ',').count() + 1;

[marcosgopen]

Hi @kikusko77 , thanks for reaching out. What is your testing scenario?
Are you testing a specific failure scenario? What is the result you would expect?
What is your setting for "lra.coordinator.urls" property?
I would guess that you are using an external coordinator and you discovered the NarayanaLRAClient "will eventually start the one from lra.coodinator.urls", which is not expected. Am I right?
cc @mmusgrov

[marcosgopen]

Looking at the code (see here https://github.com/jbosstm/lra/blob/main/client/src/main/java/io/narayana/lra/client/internal/NarayanaLRAClient.java#L254) it seems that the coordinatorUrl is set as the first reference of "lra.coordinator.urls" even if the NarayanaLRAClient is created with the url of a coordinator which is not the first of the "lra.coordinator.urls" list (e.g. 'NarayanaLRAClient("http://localhost:49101/lra-coordinator/lra-coordinator");'
@kikusko77 do you have a reproducer?

[marcosgopen]

Please notice NarayanaLRAClient is an internal class, so its use should be internal and for testing only.
At the moment it is recommended to use the "lra.coordinator.urls" property to specify the coordinator url and not the NarayanaLRAClient() constructor argument. I see the problem when more then one coordinator is specified in "lra.coordinator.urls". I will propose a solution soon.

[mmusgrov]

I agree with Marco that it is an internal API (the package name includes the component internal and when we add support for JPMS, the Java Platform Modules System, we will formalise it) but @kikusko77 if intention is to start a coordinator on endpoint "http://localhost:49101/lra-coordinator/lra-coordinator", then why not either

• include it in the lra.coordinator.urls config property, or
• make sure the lra.coordinator.urls config property is unset or empty

And even better is a third, and best, option which would be for you to contribute requirements to the issue tracker: https://issues.redhat.com/browse/JBTM-3675 "Create new LRA programmatic client API". I say it is the best option because as developers of the internals we may miss some good use cases and no doubt yours is a possible example of such a use case.

[xstefank]

@marcosgopen @mmusgrov

@kikusko77 is actually working on a new master thesis that aims to provide a full HA solution for the LRA coordinator. So he is intentionally using internal classes because his solution is integrated directly into this repository's code.

About the issue at hand, even if the class is internal, it still should adhere to the expectations of the programmer. This is the first time I see that the config property takes precedence over a directly passed value as a parameter to the constructor. Can you explain why that would make sense? In my opinion, this issue is valid and the constructor parameter should take precedence over the lra.coordinator.urls property.

[tomjenkinson]

@xstefank I don't think it was clear that @kikusko77 was looking to propose something in the core of Narayana LRA and that helps clarify the intention of the question.

The options provided look to be those that could be offered if the person was looking to use Narayana rather than develop with it.

As such, taking it from the point of view of a developer coding Narayana LRA it does seem either a useful aspect if developer has the ability to override the config, or possibly all the ability to set the config programatically for coordinatorUrl is removed - @marcosgopen do you have a preference?

BTW, the defaulting to the configured properties looks to have been added as part of #50

[marcosgopen]

As such, taking it from the point of view of a developer coding Narayana LRA it does seem either a useful aspect if developer has the ability to override the config, or possibly all the ability to set the config programatically for coordinatorUrl is removed - @marcosgopen do you have a preference?

I don't have a preference. As the class is deprecated maybe removing that ability makes more sense?
However I am going to work on #113 . I encourage people interested to talk about it in our Zulip channel: https://narayana.zulipchat.com

[tomjenkinson]

As this is a developer internal API, it seems clearer to discuss it developer-to-developer on Zulip. But as I understand it though, a developer that wants to set coordinator URL then rather than using the parameter to the NarayanaLRAClient constructur, they rather need to (programmatically) set the config property "lra.coordinator.urls" (which would override any lra.coordinator.urls that the user provides in their config file).

However, if a developer (e.g. in this case @kikusko77) needs to address this change early then please feel free to raise a pull request. @kikusko77 if you can do this sooner that would be great. If you do provide a pull request with this, please can you redefine this issue to describe the intended behaviour and provide a unit test in your pull request to show it works as expected.

[xstefank]

@kikusko77 asked me to take this off his hands as he is busy with the thesis work. I'll open a PR changing this behavior.

Also, @marcosgopen is there a topic on zulip about the new client API? One of the points should be, that it should be public and usable by users.

[marcosgopen]

@kikusko77 asked me to take this off his hands as he is busy with the thesis work. I'll open a PR changing this behavior.

Also, @marcosgopen is there a topic on zulip about the new client API? One of the points should be, that it should be public and usable by users.

I haven't created a topic for it on zulip yet, but we can create one and discuss there. I can see we discussed about the issue in many places, so limiting the discussion on a single channel is better:
Discussion:

• Create new LRA programmatic client API #113
• https://issues.redhat.com/browse/JBTM-3675
• https://issues.redhat.com/browse/JBTM-3303