test(search): surface the real cause behind ranking-IT poll timeouts - #30250
Conversation
…poll errors SearchEntityRankingIT's "Tier1 > untagged on a text tie" case seeds two documents with an identical displayName so their text scores are equal, then asserts the Tier.Tier1 global term boost breaks the tie. The tie was not actually exact. RankingSupport.uniqueTerm() derived its token from TestNamespace.uniqueShortId(), which is RUN_ID.substring(0,8) + methodHash + 4 random chars — only 4 of 16 characters vary within a test method. Every token minted for one test therefore shared a 14-character prefix, and EntitySeeder.nameFor() places a sibling token in each entity's name, so the query ngram-matched both documents' names by differing amounts. That leaked a variable name.ngram / displayName.ngram contribution into scores the case requires to be equal. Measured against 1.13 (Tier1 boost 0.05, multiplier 1.05x): 3/25 inversions. The noise ratio is bounded by roughly 1.17 because the ngram terms sit in the "max plus 0.3 times others" bucket while displayName.keyword dominates, so main's post-#29903 boost of 0.5 (1.5x) sits above the noise and never inverts — the defect is present on main too, just out of range. Drawing the token from its own full-entropy source removes the overlap at source rather than relying on a boost margin to outweigh it. Also stop reporting every awaitTrue timeout as "not indexed in time". ignoreExceptions() is correct while polling a document that is still being indexed, but discarding the exception made a condition that threw on every poll indistinguishable from genuine indexing lag. awaitOrReason() keeps the tolerance and reports the last error alongside the timeout. RankingSupportTest covers both collision properties; it fails with a 14-char shared prefix against the previous implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
🟡 Playwright Results — all passed (21 flaky)✅ 4531 passed · ❌ 0 failed · 🟡 21 flaky · ⏭️ 95 skipped
🟡 21 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
#30120 ("Stabilize integration-test CI") landed a complementary fix for the same ranking-tie flakiness this branch targeted: it makes the filler values in seeded entity names use a disjoint alphabet (EntitySeeder.tokenFreeValue = "aa"+hex) so they cannot ngram-match the "zz"+[g-v] query token. That supersedes this branch's uniqueTerm() full-entropy change, so the merge resolves to #30120's token approach (theirs) and drops the now-redundant uniqueTerm rewrite and RankingSupportTest. What remains from this branch is the piece #30120 did not touch: awaitOrReason(), which surfaces the last polling error instead of reporting every awaitTrue timeout as a flat "not indexed in time". awaitTrue() is retained for the settings tests.
mohityadav766
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @@ -110,14 +110,14 @@ private void evaluateCase( | |||
| String term = RankingSupport.uniqueTerm(ns); | |||
There was a problem hiding this comment.
⚠️ Bug: Merge reverts the entropy fix — token collision reintroduced
The merge commit (9f26c7d, the commit under review) reverted uniqueTerm from the branch's full-entropy source (UUID.randomUUID()) back to ns.uniqueShortId() and deleted its guarding test RankingSupportTest.java. Per the PR's own analysis, uniqueShortId() = RUN_ID.substring(0,8) + methodHash + 4 random chars, so within a single test method only 4 of 16 characters vary and every minted token shares a ~14-char prefix. Since seeders place a sibling uniqueTerm in each entity's displayName/name, a query for one case's token ngram-matches another case's document, leaking a variable displayName.ngram/name.ngram contribution into scores the tie-break case requires to be exactly equal — the exact ~12% inversion flakiness this PR set out to eliminate. The merge message claims main's #30120 token approach makes the rewrite "redundant", but TestNamespace.uniqueShortId() at HEAD still only varies in its last 4 chars, so the collision persists and no test remains to catch it. Restore the full-entropy token (and ideally the collision-property test).
Restore the full-entropy, method-independent token source so sibling tokens minted in the same test method no longer share a long prefix. Revert the callers in SearchEntityRankingIT to uniqueTerm() (no ns arg), re-add the UUID import and TERM_HEX_LENGTH constant, and consider restoring RankingSupportTest.:
static String uniqueTerm() {
String hex = UUID.randomUUID().toString().replace("-", "").substring(0, 16);
StringBuilder term = new StringBuilder("zz");
for (int i = 0; i < hex.length(); i++) {
term.append((char) ('g' + Character.digit(hex.charAt(i), 16)));
}
return term.toString();
}
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
Describe your changes:
Surface the real cause behind
SearchEntityRankingITpoll timeouts.RankingSupport.awaitTruereports every timeout as a flat"not indexed in time".ignoreExceptions()is correct while a document is still being indexed, but discarding the exception makes a condition that throws on every poll — a 4xx, a bad index name, a deserialization error — indistinguishable from genuine indexing lag. Both surface only as "not indexed in time", which is exactly the message dominating several ranking failures in the collate CI runs that prompted this.awaitOrReason()keeps the transient-error tolerance but retains and reports the last polling error alongside the timeout;SearchEntityRankingIT's two call sites now include it in their failure text.awaitTrue()is retained unchanged for the settings tests that legitimately want a boolean skip.Net change over
main:+awaitOrReasoninRankingSupport, andawaitTrue → awaitOrReasonat the twoSearchEntityRankingITsites. Nothing else.How I tested
openmetadata-integration-testsagainst the resolved classpath: the two changed files compile clean (the only failure is a pre-existing, unrelatedSearchIndexRetryQueueITthat needs a freshly-builtopenmetadata-service, which CI provides).1.13.Type of change:
Checklist:
Greptile Summary
This PR improves timeout diagnostics in search ranking integration tests. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile