palomar (search) iteration #263
Conversation
bnewbold
force-pushed
the
bnewbold/palomar-iterate
branch
from
365962a to
9174fd5
Compare
bnewbold
force-pushed
the
bnewbold/palomar-iterate
branch
from
777ef41 to
cafef6e
Compare
This can be left to the configured default for the index; default is 1sec. The exception is post deletions, which we process immediately.
search/firehose.go
Outdated
| if cur != 0 { | ||
| u.RawPath = fmt.Sprintf("cursor=%d", cur) | ||
| } |
There was a problem hiding this comment.
A cursor of 0 asks for the farthest back event that the host is willing to provide. Maybe we want to default the value to -1 as s sentinel so we can allow for an explicit 0 cursor?
Doesn't need to be blocking but just a thing to think about.
There was a problem hiding this comment.
We'll probably want to be able to configure this, the same way Kafka lets you default a consumer group to "start at current offset", "start at earliest available", "start at a configured time delta", etc.
For now it is in the "zero means empty/nil" situation. I'm going to leave as-is, which I think is a sane default with the backfill stuff now handling broader enumeration of backfill work.
There was a problem hiding this comment.
Just clarifying, this means if you provide no cursor we'll start from as far back as the BGS/PDS will allow us, not from live.
There was a problem hiding this comment.
Looking at this again, I guess I had a typo and it should be RawQuery not RawPath.
But if we don't provide a cursor HTTP query param at all, shouldn't the PDS (or BGS) give us the current stream, not start at the beginning? The intent of this snippet of code is to not set the parameter at all, instead of defaulting to cursor=0 (which would scroll back to oldest available).
There was a problem hiding this comment.
Oh oops yeah you're right, I read that wrong
| for _, r := range resp.Hits.Hits { | ||
| var doc PostDoc | ||
| if err := json.Unmarshal(r.Source, &doc); err != nil { | ||
| return nil, fmt.Errorf("decoding post doc from search response: %w", err) |
There was a problem hiding this comment.
Do we want this to fail the entire query or just log it with some context and move to the next result?
There was a problem hiding this comment.
This is a pretty bad error which should "never" happen, so I think we want to fail-and-bail on this situation.
An example where this would happen is if there was a config mistake where a query worker got pointed at the wrong elasticsearch index (either post instead of profile, or an older schema version). I think in that kind of situation we want to fail loudly.
|
|
||
| did, err := syntax.ParseDID(doc.DID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DID in indexed document: %w", err) |
There was a problem hiding this comment.
same as above: this is a bad and very unexpected situation
| for _, r := range resp.Hits.Hits { | ||
| var doc ProfileDoc | ||
| if err := json.Unmarshal(r.Source, &doc); err != nil { | ||
| return nil, fmt.Errorf("decoding profile doc from search response: %w", err) |
There was a problem hiding this comment.
Similarly, do we want to error here or log it and keep going?
There was a problem hiding this comment.
same as above: this is a bad and very unexpected situation
|
|
||
| did, err := syntax.ParseDID(doc.DID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DID in indexed document: %w", err) |
|
Addressed a bunch of these. The main outstanding thing is failing hard on some of the query error cases. I think that is the right thing to do, but could be wrong and we could get bitten. I'll probably merge this later/tomorrow after thinking a bit more. |
Larger refactors in this branch:
go:embedschemas)log/slog, including echo integrationatproto/identitypackage for identity caching and handling, notUserdatabase recordanalysis-icuplugin for (hopefully) better internationalized searchfrom:filteringThis branch includes a couple small commits to SDK code, which i've cherry-picked out as separate PRs for easier review.
See also Lexicon PR in atproto repo: bluesky-social/atproto#1594
This is not compatible with the previous version of
palomarat the HTTP API, opensearch index, or database schema levels. The config vars should be backwards compatible. The operational plan for staging and prod is to deploy this as an entirely new environment (eg, "prod2", "staging2"), get everything backfilled, and then flip over the AppView and then client app to use the lexicons/endpoints instead of the older version.I think this is ready for review, merge, and deploy to staging. Some things to check before prod:
Out of scope for this PR:
created_attimestamp not being reliable, by adding asort_athybrid field, for future "sort by date"