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

Bloodhound / Opensearch #4261

Draft
wants to merge 37 commits into
base: develop
Choose a base branch
from
Draft

Conversation

supersven
Copy link
Contributor

@supersven supersven commented Sep 20, 2024

This should only be released in combination with an infrastructure migration from ElasticSearch 6 to OpenSearch 1.3.

Ticket: https://wearezeta.atlassian.net/browse/WPB-10715

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@supersven supersven added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 20, 2024
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 2 times, most recently from 7669b04 to 4b643a3 Compare October 1, 2024 10:37
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 2 times, most recently from f331e51 to 1f6823e Compare October 15, 2024 14:50
@echoes-hq echoes-hq bot added the echoes: technical-debt Changes intended at mitigating risks label Oct 16, 2024
updateTeamSearchVisibilityInboundImpl :: forall r. (Member (Embed IO) r) => IndexedUserStoreConfig -> TeamId -> SearchVisibilityInbound -> Sem r ()
updateTeamSearchVisibilityInboundImpl cfg tid vis =
void $ runInBothES cfg updateAllDocs
where
updateAllDocs :: ES.IndexName -> ES.BH (Sem r) ()
updateAllDocs idx = do
r <- ES.updateByQuery idx query (Just script)
r <- hoistBH (embed @IO) $ ES.performBHRequest . fmap fst . ES.keepBHResponse $ ESR.updateByQuery @Value idx query (Just script)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This handling of version conflicts is probably broken here. See bitemyapp/bloodhound#295

@supersven supersven force-pushed the supersven/bloodhound-opensearch branch from 2b268b2 to 91ef5f0 Compare October 18, 2024 14:07
networks:
- demo_wire

opensearch-dashboard:
Copy link
Contributor

Choose a reason for hiding this comment

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

dashboard! neat! is there a good place to mention that so people might notice?

Comment on lines +97 to +98
# TODO: This should not refer to Sven's fork. Hope that we will get this
# upstreamed soonish or push the branch to Wire's fork.
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be done.

Comment on lines +44 to +46
if ES.isSuccess resp
then pure ()
else logAndThrow mkErr resp
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ES.isSuccess resp
then pure ()
else logAndThrow mkErr resp
unless (ES.isSuccess resp) $ logAndThrow mkErr resp

logAndThrow :: (Member TinyLog r, Member (Error MigrationException) r, Show e) => (String -> MigrationException) -> e -> Sem r a
logAndThrow mkErr errMsg = do
Log.warn $
Log.msg (Log.val ("An OpenSearch/ElasticSearch error appeared: " `BS.append` (encodeUtf8 . Text.pack . show) errMsg))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Log.msg (Log.val ("An OpenSearch/ElasticSearch error appeared: " `BS.append` (encodeUtf8 . Text.pack . show) errMsg))
Log.msg (Log.val ("An OpenSearch/ElasticSearch error occurred: " `BS.append` (encodeUtf8 . Text.pack . show) errMsg))

case persistResponse of
Left _ -> throw $ PersistVersionFailed v $ show persistResponse
Right r ->
if ES.idxDocId r == docIdText
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this still do something involving ES.isCreated?

Comment on lines +139 to +140
castResponse :: forall context1 val1 context2 val2. BHResponse context1 val1 -> BHResponse context2 val2
castResponse BHResponse {..} = BHResponse {..}
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? How does this work without casting the fields?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-debt Changes intended at mitigating risks ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants