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 8 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-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nix/haskell-pins.nix Outdated Show resolved Hide resolved
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's about casting phantom types. However, I've refactored this part to make it better understandable (Your question was an important hint!)

-- | Parse `BHResponse` as `ES.EsError`
parseESError :: BHResponse ES.StatusDependant any -> Either ES.EsProtocolException ES.EsError
parseESError res = either id id <$> ES.parseEsResponse (castResponse res)
  where
    castResponse :: BHResponse ES.StatusDependant any -> BHResponse ES.StatusDependant ES.EsError
    castResponse BHResponse {..} = BHResponse {..}

(https://github.com/wireapp/wire-server/pull/4261/files#diff-602f14de0098d6204cc320e8563551188a5e102e48ac6b2a68150811a13f8437R208)

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

just rejecting this PR so nobody accidentally merges it (see description).

@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 3 times, most recently from 9c29e8c to 8eb8c42 Compare November 11, 2024 14:20
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 2 times, most recently from a21d71b to b58a7d5 Compare November 18, 2024 08:32
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 2 times, most recently from d1a5104 to ad27cff Compare November 25, 2024 08:43
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch 2 times, most recently from 857328d to 1230a33 Compare November 25, 2024 17:13
The latest master of Bloodhound contains a fix regarding parsing
responses that we need to use OpenSearch 1.x.

Unfortunately, the interface of this library changed a lot.
This will be our new target, to be used on-prem and in the Wire cloud.
This includes adding the OpenSearch Dashboards (for better visibility
and debugging purposes.)
This reflects the prior behaviour.
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch from 1230a33 to abed28e Compare December 11, 2024 16:52
Won't be compatible to OpenSearch. And, users should setup their
services on their own.
This fits better into the Parser picture: If a value is invalid, fail in
parsing - don't throw errors later.
@supersven supersven force-pushed the supersven/bloodhound-opensearch branch from abed28e to 46cf9ea Compare December 12, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. 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