Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Upgrade Conductor to Spring 3 #3825

Closed
wants to merge 31 commits into from

Conversation

LuisLainez
Copy link
Contributor

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • [X ] 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

Upgrading Conductor to Spring 3 - WARNING - Server doesn't start successfully.
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 #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.

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

Alternatives considered

Describe alternative implementation you have considered

@LuisLainez LuisLainez changed the title Issue/upgrade to spring Upgrade Conductor to Spring 3 Oct 27, 2023
@LuisLainez LuisLainez marked this pull request as draft October 27, 2023 04:05
@manan164
Copy link
Contributor

Hi @LuisLainez , Thanks for the initiative. I am able to bring up the server using hard-coded properties. I will investigate further why @Configuration not working properly in spring-boot3

Validation.byProvider(ApacheValidationProvider.class)
.configure()
.buildValidatorFactory();
validatorFactory = Validation.buildDefaultValidatorFactory();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to delete the ApacheValidator because bval-jsr does not allow the new jakarta stuff - and version 3x is not yet public.

@LuisLainez
Copy link
Contributor Author

LuisLainez commented Nov 3, 2023

manan164 It is solved now - the problem was that the orkes-queue lib was misbehaving - it was being loaded but the needed stuff is in the repo already.
Deleting the lib has worked
I am not really sure where the Conductor queues is being used - or what's its function.

@LuisLainez LuisLainez marked this pull request as ready for review November 3, 2023 06:15
@LuisLainez LuisLainez marked this pull request as draft November 3, 2023 06:15
@LuisLainez
Copy link
Contributor Author

LuisLainez commented Nov 14, 2023

manan164 It is solved now - the problem was that the orkes-queue lib was misbehaving - it was being loaded but the needed stuff is in the repo already. Deleting the lib has worked I am not really sure where the Conductor queues is being used - or what's its function.

Deleting the orker-queue library is not a solution, as it'd be needed for conductor.

The problem of server not starting up is that it blows up on io.orkes.conductor.queue.config.RedisQueueConfiguration as the properties that should have been initialized in QueueRedisProperties are not being populated.
Strangely enough the com.netflix.conductor.redis.config.RedisProperties that lives in the conductor repo is correctly populated - and that blows up in
io.orkes.conductor.queue.config.ConfigurationHostSupplier::parseHostsFromConfig

@LuisLainez
Copy link
Contributor Author

LuisLainez commented Nov 14, 2023

Further updated - once this PR is merged and the library is published - the configuration will be picked up and server will start (tested in local with local maven)

cc @manan164

orkes-io/orkes-queues#13

@LuisLainez LuisLainez closed this Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants