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

Use dual indexer and separate consumers for OpenSearch migration #6559

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

neil-xie
Copy link
Member

What changed?
Added dual indexer which contains 2 indexer to handle write operation to ES/OS
Each indexer will has its own consumer, so ES and OS can ack/nack kafka messages independently
Remove the migration indexer

Will land this after the changes in monorepo

Why?
OpenSearch migration

How did you test it?
Manual test to verify
Unit test

Potential risks

Release notes

Documentation Changes

service/worker/indexer/esProcessor.go Show resolved Hide resolved
logger.Fatal("Secondary Index ES processor state changed", tag.LifeCycleStartFailed, tag.Error(err))
}

secondaryConsumer, err := client.NewConsumer(common.VisibilityAppName, getConsumerName(secondaryVisibilityName+"-os"))
Copy link
Member

Choose a reason for hiding this comment

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

it would be better to define consumer names in the config. secondaryVisibilityName+"-os" looks a bit hidden behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah,I will update it. I am also not sure if OS should always use the name pattern *-os-consumer, since this can help when the migration is done and this extra consumer for OS will become the primary. Or we need to rename the consumer group to *-consumer to match the existing code.

Comment on lines +408 to +415
if err := visibilityDualIndexer.SourceIndexer.Start(); err != nil {
visibilityDualIndexer.SourceIndexer.Stop()
s.GetLogger().Fatal("fail to start source indexer", tag.Error(err))
}

if err := visibilityDualIndexer.DestIndexer.Start(); err != nil {
visibilityDualIndexer.DestIndexer.Stop()
s.GetLogger().Fatal("fail to start dest indexer", tag.Error(err))
Copy link
Member

Choose a reason for hiding this comment

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

service doesn't need to know these source/dest indexers. the interface could stay the same from service perspective and the migration-dual-indexer can start/stop these

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