Document ES7 indexing type and add condition regression test#929
Document ES7 indexing type and add condition regression test#929bulgarian-beast wants to merge 5 commits intoconductor-oss:mainfrom
Conversation
Docker ES7-backed configs enabled indexing and set Elasticsearch version/url, but did not set conductor.indexing.type=elasticsearch. Because the ES7 IndexDAO condition requires that property, the MySQL docker-compose example could fail at startup with a missing IndexDAO bean. This change: - adds conductor.indexing.type=elasticsearch to ES7 docker configs - updates the docs to show the required property - adds a regression test for the ES7 condition
da624f6 to
2896bc3
Compare
|
Would you mind taking a look at this PR when you have a moment? This ES7 Docker config issue has been pretty disruptive on my side because it breaks the MySQL/Redis/ES7 local stack and I end up hitting it again whenever I branch from main before the fix is merged. I kept the scope intentionally small:
Thanks a lot. |
nthmost-orkes
left a comment
There was a problem hiding this comment.
Thanks for the fix — the doc change and the regression test are both clearly in scope, but the PR as it stands is confusing to review because the description and the diff don't match. Requesting changes purely to reconcile that before merge.
What's actually in the diff
Only two files:
docs/devguide/running/deploy.md— addsconductor.indexing.type=elasticsearchto the ES7 example.es7-persistence/src/test/java/com/netflix/conductor/es7/config/ElasticSearchConditionsTest.java— new regression test.
What the description claims, but isn't in the diff
The PR body says it modifies four Docker config files:
docker/server/config/config-mysql.propertiesdocker/server/config/config-redis.propertiesdocker/server/config/config-postgres-es7.propertiesdocker/server/config/config.properties
On current main, the three non-comment configs above already contain conductor.indexing.type=elasticsearch — they were added via #900 (e2e test suite). So those changes aren't needed here, but the description still advertises them as part of this PR.
Could you either:
- Rebase onto
main, update the title/body to reflect the real scope (docs fix + regression test), and confirm whether #928 is still open against the Docker configs or only against the documented example, or - If you intended additional config changes beyond what's on
main, push them — right now they're not in the branch.
Test quality — LGTM once scope is reconciled
ElasticSearchConditionsTest is well-designed:
- Uses Spring Boot's
ApplicationContextRunner, which is the idiomatic way to test@Conditionalclasses without spinning up a full context. - Five cases cover the meaningful branches of
ElasticSearchV7Enabled: happy path, implicit version (relies onmatchIfMissing=true), disabled indexing, missingconductor.indexing.type(the exact regression this PR is preventing), and wrong-type selector (elasticsearch8). - Follows the JUnit 4 convention of the sibling
ElasticSearchPropertiesTestin the same package. @Configuration(proxyBeanMethods = false)is the right call for a lightweight test config.
Minor nits (non-blocking)
shouldActivateWhenVersionIsImplicitlocks in the currentmatchIfMissing=truebehavior forconductor.elasticsearch.version. Worth a one-line comment so a future maintainer who tightens that condition doesn't have to reverse-engineer the expectation.static class Marker {}→Es7Markerwould read slightly better alongside the"es7Marker"bean name. Trivial.
Doc change — LGTM
The deploy.md addition is correct: conductor.indexing.type has no matchIfMissing on its @ConditionalOnProperty in ElasticSearchConditions.java, so users who follow the documented snippet verbatim really do need this line.
|
Thanks for the detailed review. I rebased/fast-forwarded the branch and reconciled the scope with current main. The Docker config files already contain conductor.indexing.type=elasticsearch via #900, so this PR is now intentionally limited to:
I also addressed the non-blocking test nits by adding a short comment for the implicit ES7 version behavior and renaming Marker to Es7Marker. Verified again with: ./gradlew spotlessApply |
|
Hello @nthmost-orkes, The failing redis-es8 e2e job appears unrelated to this PR. This branch only changes the ES7 deployment doc snippet and adds ES7 conditional activation coverage. The failed job is in the Redis + ES8 e2e matrix and the visible logs point to sync workflow timing/e2e behavior. I re-ran the focused verification locally: ./gradlew spotlessApply |
Thanks, revisiting this today! |
Summary
Fix ES7 Docker example configs and docs to explicitly select Elasticsearch indexing by setting:
conductor.indexing.type=elasticsearchWithout this property, the ES7
IndexDAOcondition does not match, which can cause server startup to fail because noIndexDAObean is created.Fixes #928.
Pull Request type
What was wrong
The ES7-backed Docker configs already enabled indexing and set the Elasticsearch version/URL, but a property was not set:
conductor.indexing.type=elasticsearchThat property is required for the ES7
IndexDAOconfiguration to activate.Because of that, all Docker setups fail during startup with a missing
IndexDAObean, for example:I reproduced this with:
Although I hit the issue through the MySQL compose path, the same missing property existed in the ES7-backed Docker configs more broadly, so this PR fixes the configuration consistently across those examples.
What this PR changes
Config fixes
Adds:
conductor.indexing.type=elasticsearchto:
docker/server/config/config-mysql.propertiesdocker/server/config/config-redis.propertiesdocker/server/config/config-postgres-es7.propertiesDocumentation fixes
Updates ES7 examples in:
docker/server/config/config.propertiesdocs/devguide/running/deploy.mdso the documented configuration matches the actual runtime requirement.
Regression coverage
Adds a regression test for the ES7 conditional activation:
es7-persistence/src/test/java/com/netflix/conductor/es7/config/ElasticSearchConditionsTest.javaWhy this fix is needed
The ES7 persistence module does not activate its
IndexDAObased only on:conductor.indexing.enabled=trueconductor.elasticsearch.version=7It also requires the indexing backend to be explicitly selected with:
conductor.indexing.type=elasticsearchWithout that property, Spring does not create the ES7
IndexDAO, and server startup can fail before initialization completes.Verification
Ran:
Verified:
spotlessApplycompleted successfullyconductor-serverreachedhealthystateScope
This PR is intentionally narrow: