Skip to content

Feature: Add support for ES8 Persistence#739

Open
rajeshwar-nu wants to merge 51 commits intoconductor-oss:mainfrom
steeleye:es8-persistence
Open

Feature: Add support for ES8 Persistence#739
rajeshwar-nu wants to merge 51 commits intoconductor-oss:mainfrom
steeleye:es8-persistence

Conversation

@rajeshwar-nu
Copy link
Contributor

@rajeshwar-nu rajeshwar-nu commented Jan 28, 2026

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

Add support for ES8 persistence
Should help with plan #678

@rajeshwar-nu rajeshwar-nu marked this pull request as ready for review January 28, 2026 14:01
@v1r3n
Copy link
Collaborator

v1r3n commented Feb 3, 2026

Hi @rajeshwar-nu

  1. can you fix the tests?
  2. Can you move the packages to org.conductoross.conductor

@rajeshwar-nu
Copy link
Contributor Author

@nthmost-orkes can I get some reviews on this please, tests are passing, and pr conflicts are resolved

@nthmost-orkes
Copy link
Contributor

Thanks for this — the ES8 module itself is well-structured and the new Java API client approach is the right direction. A few things worth addressing before merge:

Core changes should be a separate PR

WorkflowSweeper.java (+92/-13) and ExecutorUtils.java (+51/-36) contain substantive bug fixes that are unrelated to ES8 persistence. Specifically:

  • Removal of .parallel() from the sweeper loop — this is a significant behavioral change with no rationale in the PR description
  • New sub-workflow stuck-task repair logic (repairSubWorkflowTask)
  • Lock-miss behavior changed from a warn log to backoff-and-requeue
  • filter(isTaskRepairable) predicate removed without explanation

These look like real fixes, but bundling them into an ES8 PR makes both harder to review and adds unnecessary risk. Would you be willing to split them out into a separate PR?

ElasticSearchConditions uses the legacy two-property pattern

@ConditionalOnProperty(name = "conductor.elasticsearch.version", havingValue = "8")

The newer OpenSearch modules (os-persistence-v2, os-persistence-v3) use conductor.indexing.type as the single backend selector, which is the direction issue #678 is pushing toward. It would be more consistent to check conductor.indexing.type=elasticsearch8 here, rather than continuing the legacy two-property pattern (indexing.type=elasticsearch + elasticsearch.version=8).

Minor

  • docker-compose-es8.yaml has a top-level version: field — this is obsolete in Compose v2 and triggers a warning. Safe to remove.
  • The ES image is pinned to 8.12.2 — current is 8.17.x. Not a blocker, but worth updating before merge.

@nthmost-orkes
Copy link
Contributor

Update after local testing:

Found a blocking startup bug: the elasticsearch-java client is pinned to 8.19.11 in dependencies.gradle, but docker-compose-es8.yaml ships ES 8.12.2. The 8.19 client calls /_cluster/health and requires the unassigned_primary_shards field in the response, which was only added in ES 8.16. Running against 8.12 causes an immediate crash:

co.elastic.clients.util.MissingRequiredPropertyException: 
  Missing required property 'HealthResponse.unassigned_primary_shards'

Fix: bump the docker-compose image to at least 8.16.x. Tested with 8.17.0 — server starts cleanly, all indices created (conductor_workflow-000001, conductor_task-000001, etc.), workflow execution and ES search work correctly.

The compose file should pin a version compatible with the client, or the client version should be documented as the minimum required ES version.

@rajeshwar-nu
Copy link
Contributor Author

@nthmost-orkes Appreciate your earlier review, I have addressed all the comments. Can I get another review please

@nthmost-orkes
Copy link
Contributor

Thanks for working through all the feedback — the updates look solid:

  • ✅ Core changes (WorkflowSweeper, ExecutorUtils) removed from this PR
  • ElasticSearchConditions now uses conductor.indexing.type=elasticsearch8 (matches the os-persistence-v2/v3 pattern)
  • version: field removed from docker-compose
  • ✅ ES image bumped to 8.19.11 to match the Java client, with a comment explaining the alignment — good call

Config properties look clean, the new Java API client approach is the right direction for ES8, and packages are correctly under org.conductoross.conductor.

This is ready for merge from my side. @v1r3n for final approval.

@rajeshwar-nu
Copy link
Contributor Author

Hey @v1r3n , could you please take a look at this, would appreciate if we can merge this soon 🙏🏻

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.

3 participants