Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/upgrade to spring (#3828) #1

Merged
merged 8 commits into from
Dec 17, 2023
Merged

Issue/upgrade to spring (#3828) #1

merged 8 commits into from
Dec 17, 2023

Conversation

v1r3n
Copy link
Collaborator

@v1r3n v1r3n commented Dec 8, 2023

SB3 upgrades

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #

Alternatives considered

Changes in this PR

Upgrading Conductor to Spring 3
The main changes are:

Upgrade Spring version to 3.1.4
Add org.elasticsearch.client is not in the bom - has to be self managed
Groovy + Spock - Version 4.0.9 (changed from org.codehaus to org.apache )
Add com.google.googlejavaformat as it was not in bom
WorkflowDefConstraintTest -> Need to delete the ApacheValidator because bval-jsr does not allow the new jakarta stuff - and version 3x is not yet public.
There is an issue with grpc libraries - they generate the ‘Generated’ tag in javax - and then it can’t compile. As per the thread here protobuf-maven-plugin supports jakarta instead of deprecated javax.annotation · Issue Netflix/conductor#1113 · grpc/grpc.io , we will need to also add the javax annotation - we should not use it inside the project though: implementation 'javax.annotation:javax.annotation-api:1.3.2'
In HTTPTask , the method field used to be HTTP Method - which was an enum and serialized fine, it is no longer an enum and serializes bad - I am decoupling the internal model from the Spring Framework so it doesn’t break constantly.
I had to add the dependency Maven Repository: org.apache.httpcomponents.client5 » httpclient5 » 5.2.1 because it’s needed in org.springframework.http.client
Had to add the explicit jakarta annotations in grpc-client as it depends on grpc and that has the javax as well - just didnt compile.
In DefaultRestTemplateProvider , the method setReadTimeout is now deprecated and no longer has an effect - so we had to do changes in there. We are no longer user a restTemplate in the constructor, but a builder, and we set up the timeout in there taking into account the input from the task. Alert: This will create more RestTemplates potentially in the thread, but it’s the only way we can set up the timeout PER request. An alternative solution is to create 1 default one and then a custom one per request.
Use GRPC version 1.57.2 - Since that works, keeping it 1+ caused compatibility issues with protobuf.
Updated orkes queues to new 1.0.7 version compatible with Spring3
Next steps - not required for this release :

Upgrade Jedis to 4.X + as the version 3.6.0 is not compatible with Spring 3.1.5. Keeping 2.7.16 in that redis-persistence-module
Upgrade libraries for ApacheValidator - bval-jsr version 3.X +

@LuisLainez
Copy link
Contributor

Thanks for continuing my original work in this new repo Netflix/conductor#3855 team!- I would advise to paste the description of the original PR - which explains all the changes and steps forward.

@v1r3n
Copy link
Collaborator Author

v1r3n commented Dec 17, 2023

Thanks for continuing my original work in this new repo Netflix/conductor#3855 team!- I would advise to paste the description of the original PR - which explains all the changes and steps forward.

Done.

@LuisLainez can you review the PR to ensure there are no missing files or updates?

@v1r3n v1r3n merged commit fb122cb into main Dec 17, 2023
4 checks passed
@v1r3n v1r3n deleted the springboot3_upgrade branch December 17, 2023 19:08
@LuisLainez
Copy link
Contributor

Looks fine to me - i have added a following PR with a couple of cleanups I forgot to do (big PR, apologies)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants