Skip to content

Work around imprecisions in ES result ordering#1776

Closed
pcapriotti wants to merge 3 commits intodevelopfrom
pcapriotti/search-order
Closed

Work around imprecisions in ES result ordering#1776
pcapriotti wants to merge 3 commits intodevelopfrom
pcapriotti/search-order

Conversation

@pcapriotti
Copy link
Contributor

This sorts the result list after the fact by putting exact handle and name searches at the beginning, so we don't have to rely on ES being precise with the ordering of exact matches.

Hopefully this fixes the flakyness of the order-name integration test, and makes #1774 unnecessary.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information:
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

This sorts the result list after the fact by putting exact handle and
name searches at the beginning, so we don't have to rely on ES being
precise with the ordering of exact matches.

Hopefully this fixes the flakyness of the `order-name` integration test,
and makes #1774 unnecessary.
@jschaul
Copy link
Member

jschaul commented Sep 16, 2021

Multiple search tests, both local and for federation fail. So this change seems to break things. CI has detailed logs.

let maxResults = maybe 15 (fromIntegral . fromRange) maybeMaxResults
teamSearchInfo <- mkTeamSearchInfo

maybeExactHandleMatch <- exactHandleSearch teamSearchInfo
Copy link
Member

Choose a reason for hiding this comment

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

There is a problem here: whether or not results are returned for users depends on the searchable flag (team users are by default not searchable by prefix search, but only by exact-handle-in-cassandra-search). The changeset here by no longer doing a request to cassandra omits those results entirely. That might at least partially explain the test failures.

The logic is a bit weird and grew as more requirements were tacked on to the product.

@pcapriotti
Copy link
Contributor Author

On second thought, this approach is completely flawed, because sorting of results has to happen before the limit is applied. This should either be fixed at the Lucene level, or ignored. Closing this.

@pcapriotti pcapriotti closed this Sep 17, 2021
@pcapriotti pcapriotti mentioned this pull request Sep 21, 2021
4 tasks
@fisx
Copy link
Contributor

fisx commented Sep 21, 2021

ah, just saw that you didn't merge this. good, as it explains that it just happened again here:

    order-name (prefix match)
      new-index:                                                                {"user":"5662c79e-fdff-4a45-a818-bb357820e507","request":"N/A","msgs":["I","Creating user"]}
FAIL (0.53s)
        test/integration/API/Search.hs:271:
        Expected order: name match, name prefix match.

        Since this test fails sporadically for unknown reasons here is some debug info:
        results: [Contact {contactQualifiedId = Qualified {qUnqualified = de62b433-a095-4622-8d61-7591d72d4f14, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}}, contactName = "\38756\14782\33424\10220\340\31570\42839\26631\7813\5172\41999\4647\6937\25202\65079\25037\46105\63865\38039\32619\54158\55037\48600\37989\47721\4385\49700\54261\25054\51918\39871\15538\660\46602\52491\20261\23526\31008\26832\52943\30233\32760\53460\51722\4222\5074\10193\5741\11150suffix", contactColorId = Just 0, contactHandle = Nothing, contactTeam = Nothing},Contact {contactQualifiedId = Qualified {qUnqualified = a4544af8-5c7a-448a-bc8c-d8ccdb5f4161, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}}, contactName = "\38756\14782\33424\10220\340\31570\42839\26631\7813\5172\41999\4647\6937\25202\65079\25037\46105\63865\38039\32619\54158\55037\48600\37989\47721\4385\49700\54261\25054\51918\39871\15538\660\46602\52491\20261\23526\31008\26832\52943\30233\32760\53460\51722\4222\5074\10193\5741\11150", contactColorId = Just 0, contactHandle = Nothing, contactTeam = Nothing}]
        searchedWord: 靤㦾芐⟬Ŕ筒ꝗ标ẅᐴꐏሧᬙ扲︷懍됙凉钗罫펎훽뷘鑥멩ᄡ숤폵懞쫎鮿㲲ʔ똊촋伥實礠棐컏瘙翸탔쨊ၾᏒ⟑᙭⮎
        expected: [Qualified {qUnqualified = a4544af8-5c7a-448a-bc8c-d8ccdb5f4161, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}},Qualified {qUnqualified = de62b433-a095-4622-8d61-7591d72d4f14, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}}]
         but got: [Qualified {qUnqualified = de62b433-a095-4622-8d61-7591d72d4f14, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}},Qualified {qUnqualified = a4544af8-5c7a-448a-bc8c-d8ccdb5f4161, qDomain = Domain {_domainText = "federation-test-helper.test-lq4fry6v1847.svc.cluster.local"}}]

@pcapriotti
Copy link
Contributor Author

That's right. I've now submitted #1798 to just remove this flaky test. However, as soon as I've done that, another integration test run failed because of some other flaky search order test:

        team-mates are listed before team-outsiders (prefix match):             FAIL (18.75s)
          test/integration/API/Search.hs:361:
          team mate not found in search

Looks like the way we have set things up with search, the order of ES results is just not reliable. Either we play around with boosts, or we make these tests deterministic and with reasonable inputs, so that they stop flaking so much.

@pcapriotti pcapriotti deleted the pcapriotti/search-order branch April 14, 2022 11:06
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