feat UrlResolver #4546

xmgdtc · 2024-08-21T07:15:15Z

1 use UrlResolver class to replace baseUrl string in ExternalTaskClientBuilder.class so that we can control how to get the address such as use load balance

CLAassistant · 2024-08-22T20:17:09Z

All committers have signed the CLA.

danielkelemen · 2024-08-23T13:41:08Z

Hi @xmgdtc,

Thank you for opening a PR!
You need sign our CLA Contributor License Agreement in order to be able to contribute.

So this feature would make it possible to change the baseUrl dynamically without restarting the client. It makes sense, I will soon review your code changes and provide some feedback.

-Daniel

xmgdtc · 2024-08-26T01:08:51Z

i have completed and signed this agreement thank you for your review

PHWaechtler · 2024-12-04T11:53:10Z

Hi @xmgdtc,
Thank you for raising this PR, I have taken over review from daniel and will be reviewing it in the next weeks. Before that though, I noticed there is a conflict in clients/java/client/src/main/java/org/camunda/bpm/client/impl/EngineClient.java, could you please resolve this before review?
Thanks,
Helene

xmgdtc · 2024-12-05T01:45:53Z

The conflict has been resolved
Thanks

PHWaechtler

Hi again @xmgdtc,

Thanks again for your contribution! I have now reviewed it and left some comments for you. Most of them are just small spelling issues and two missing license headers, the main logical adjustment that needs made is to address the null/empty check for the baseURL here and my question regarding the correct class checked here.

Additionally to the comments, I have these requests:

You mentioned that you have signed the license agreement but that doesn't seem to have gone through as its still marked as unsigned, can you please have another look here and sign again?
Can you please add test coverage for this new implementation, note we have some testing best practices documented here

Once the comments and the above are implemented I will do another re review.
Thank you, and please let me know if you have any questions.
Helene

PHWaechtler · 2024-12-17T12:00:58Z

clients/java/client/src/main/java/org/camunda/bpm/client/ExternalTaskClientBuilder.java

@@ -34,11 +34,45 @@ public interface ExternalTaskClientBuilder {
  /**
   * Base url of the Camunda BPM Platform REST API. This information is mandatory.
   *
+   * if use this method, it will create a permanent url resolver with the given baseUrl


Small grammar improvement:

Suggested change

* if use this method, it will create a permanent url resolver with the given baseUrl

* If this method is used, it will create a permanent url resolver with the given baseUrl.

PHWaechtler · 2024-12-17T12:02:48Z

clients/java/client/src/main/java/org/camunda/bpm/client/ExternalTaskClientBuilder.java

   * @param baseUrl of the Camunda BPM Platform REST API
   * @return the builder
   */
  ExternalTaskClientBuilder baseUrl(String baseUrl);

+
+  /**
+   * url resovler of the Camunda BPM Platform REST API. This information is mandatory.


Typo:

Suggested change

* url resovler of the Camunda BPM Platform REST API. This information is mandatory.

* Url resolver of the Camunda BPM Platform REST API. This information is mandatory.

PHWaechtler · 2024-12-17T12:04:12Z

clients/java/client/src/main/java/org/camunda/bpm/client/ExternalTaskClientBuilder.java

+  /**
+   * url resovler of the Camunda BPM Platform REST API. This information is mandatory.
+   *
+   * if the server is ther cluster or you are using spring cloud. you can create a class implements UrlResolver


Grammar/Spelling improvements:

Suggested change

* if the server is ther cluster or you are using spring cloud. you can create a class implements UrlResolver

* If the server is in a cluster or you are using spring cloud, you can create a class which implements UrlResolver.

PHWaechtler · 2024-12-17T12:05:06Z

clients/java/client/src/main/java/org/camunda/bpm/client/UrlResolver.java

+package org.camunda.bpm.client;
+
+/**
+ * get service url from camunda server


Small:

Suggested change

* get service url from camunda server

* Get service url of the Camunda server

PHWaechtler · 2024-12-17T12:05:50Z

clients/java/client/src/main/java/org/camunda/bpm/client/impl/EngineClient.java

+
+
+


🔧 Nit: we can reduce these newlines

PHWaechtler · 2024-12-17T12:13:22Z

...nts/java/client/src/main/java/org/camunda/bpm/client/impl/ExternalTaskClientBuilderImpl.java

+        if (this.urlResolver instanceof PermanentUrlResolver) {
+            ((PermanentUrlResolver) this.urlResolver).setBaseUrl(sanitizeUrl(this.urlResolver.getBaseUrl()));
+        }


❓ According to the javadoc added above, I believe we're expecting urlResolver to be an instance of UrlResolver, not necessarily PermanentUrlResolver. Can we please adjust this check accordingly, or let me know if I'm misunderstanding 🤔

On branch master this method is like blow,i thought it is used for replacing "/" from the baseUrl. but when useing random loadbalance we can`t fight out all the urls. so i kept the logic, when it is a permanent address, still call this method. if not ,developer should resolve by themself

protected void initBaseUrl() {
baseUrl = sanitizeUrl(baseUrl);
}

protected String sanitizeUrl(String url) {
url = url.trim();
if (url.endsWith("/")) {
url = url.replaceAll("/$", "");
url = sanitizeUrl(url);
}
return url;
}

Ah right, so this is just the default sanitation when using the PermanentUrlResolver right?

Yes i kept this logic so that the people who using the regular baseUrl String won`t need to change anything

PHWaechtler · 2024-12-17T12:14:22Z

clients/java/client/src/main/java/org/camunda/bpm/client/impl/PermanentUrlResolver.java

+import org.camunda.bpm.client.UrlResolver;
+
+/**
+ * urlResolver with permanent address


🔧 Nit

Suggested change

* urlResolver with permanent address

* UrlResolver with permanent address

PHWaechtler · 2024-12-17T14:12:09Z

clients/java/client/src/main/java/org/camunda/bpm/client/ExternalTaskClientBuilder.java

+   *
+   * this is a sample for spring cloud DiscoveryClient
+   *
+   * public class CustomUrlResolver implements UrlResolver{


👍 Nice to have an example included here

PHWaechtler · 2024-12-17T14:20:02Z

clients/java/client/src/main/java/org/camunda/bpm/client/UrlResolver.java

@@ -0,0 +1,9 @@
+package org.camunda.bpm.client;


This file is missing the licenseHeader, can you please add it to the top of this file. You can copy it from one of the other files

PHWaechtler · 2024-12-17T14:20:13Z

clients/java/client/src/main/java/org/camunda/bpm/client/impl/PermanentUrlResolver.java

@@ -0,0 +1,24 @@
+package org.camunda.bpm.client.impl;


This file also needs the license header added to it

xmgdtc · 2024-12-19T07:13:04Z

@PHWaechtler I finished fixing spilling and licenseHeaders. sorry for my bad english. i tried to write a test method. but unfortunately i can`t download all the dependencies with my internet . so i am not sure this test works. but i try this code in my project and it is works

PHWaechtler · 2024-12-20T12:01:35Z

Hi again @xmgdtc,
Thanks for implementing the changes 🙏
Before I do the re review I noticed why the CLA license agreement is not marked as signed. I saw you are using two different accounts: the initial commit was made by mxl1909 yet the other commits and the PR was created with user xmgdtc and the CLA is not signed formxl1909. I think the easiest way to remedy this is for you to adjust the commit history on this branch to only include commits from xmgdtc (you can just squash them into one if you like) then you only have to sign the CLA with xmgdtc. Can you please do this as otherwise we cannot merge the PR without a signed CLA.

PHWaechtler · 2025-01-07T08:45:58Z

Hi @xmgdtc , just pinging again to make sure you saw this request. Please have a look so that the CLA can be signed, otherwise we cannot progress with this PR. Thans!

…ss so that we can use load balance address

# Conflicts: # clients/java/client/src/main/java/org/camunda/bpm/client/impl/EngineClient.java

xmgdtc · 2025-01-08T02:53:20Z

Hi @PHWaechtler , I rebased and commited again. Sorry for keep you waiting for so long beacuse i got a fever last week .

PHWaechtler · 2025-01-09T12:22:32Z

Hi @xmgdtc ,
no worries I hope you feel better, and thanks for signing. I reviewed the changes and found a couple more things for adjustment - since you mentioned you had issues with the test/dependencies and your internet I went ahead and applied the changes on a duplicate branch here. Since that is a new PR it will require an additional review from another member of our team, I will reach out about this internally and assign one soon. If you have any comments or questions please let me know. Once its approved we will merge the changes
Thanks again for your contribution
Helene

PHWaechtler · 2025-01-15T11:27:15Z

Hi @xmgdtc, just writing to let you know we have reviewed merged the PR with your feature so I will close this one now. Thanks for your contribution!

danielkelemen self-requested a review August 23, 2024 13:41

danielkelemen assigned xmgdtc Aug 23, 2024

PHWaechtler force-pushed the master branch from d5a4999 to 824b35f Compare December 3, 2024 12:41

tasso94 requested review from PHWaechtler and removed request for danielkelemen December 4, 2024 10:36

PHWaechtler requested changes Dec 17, 2024

View reviewed changes

xmgdtc added 2 commits January 8, 2025 10:28

1 use UrlResolver to replace baseUrl in ExternalTaskClientBuilder.cla…

a6e414c

…ss so that we can use load balance address

Merge branch 'master' into dev

a52d190

# Conflicts: # clients/java/client/src/main/java/org/camunda/bpm/client/impl/EngineClient.java

xmgdtc force-pushed the dev branch from 20e0da2 to a52d190 Compare January 8, 2025 02:36

PHWaechtler mentioned this pull request Jan 9, 2025

feat(baseUrl): add UrlResolver #4857

Merged

PHWaechtler mentioned this pull request Jan 9, 2025

ExternalTaskClientBuilder uses URLResolver for baseURL #4888

Closed

1 task

PHWaechtler closed this Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat UrlResolver #4546

feat UrlResolver #4546

xmgdtc commented Aug 21, 2024

CLAassistant commented Aug 22, 2024 •

edited

Loading

danielkelemen commented Aug 23, 2024

xmgdtc commented Aug 26, 2024

PHWaechtler commented Dec 4, 2024

xmgdtc commented Dec 5, 2024

PHWaechtler left a comment •

edited

Loading

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

xmgdtc Dec 19, 2024

PHWaechtler Jan 9, 2025

xmgdtc Jan 9, 2025

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

PHWaechtler Dec 17, 2024

xmgdtc commented Dec 19, 2024

PHWaechtler commented Dec 20, 2024

PHWaechtler commented Jan 7, 2025 •

edited

Loading

xmgdtc commented Jan 8, 2025

PHWaechtler commented Jan 9, 2025

PHWaechtler commented Jan 15, 2025

	* if use this method, it will create a permanent url resolver with the given baseUrl
	* If this method is used, it will create a permanent url resolver with the given baseUrl.

	* url resovler of the Camunda BPM Platform REST API. This information is mandatory.
	* Url resolver of the Camunda BPM Platform REST API. This information is mandatory.

	* if the server is ther cluster or you are using spring cloud. you can create a class implements UrlResolver
	* If the server is in a cluster or you are using spring cloud, you can create a class which implements UrlResolver.

	* get service url from camunda server
	* Get service url of the Camunda server

	* urlResolver with permanent address
	* UrlResolver with permanent address

feat UrlResolver #4546

feat UrlResolver #4546

Conversation

xmgdtc commented Aug 21, 2024

CLAassistant commented Aug 22, 2024 • edited Loading

danielkelemen commented Aug 23, 2024

xmgdtc commented Aug 26, 2024

PHWaechtler commented Dec 4, 2024

xmgdtc commented Dec 5, 2024

PHWaechtler left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmgdtc commented Dec 19, 2024

PHWaechtler commented Dec 20, 2024

PHWaechtler commented Jan 7, 2025 • edited Loading

xmgdtc commented Jan 8, 2025

PHWaechtler commented Jan 9, 2025

PHWaechtler commented Jan 15, 2025

CLAassistant commented Aug 22, 2024 •

edited

Loading

PHWaechtler left a comment •

edited

Loading

PHWaechtler commented Jan 7, 2025 •

edited

Loading