Fixes for replicated query iterator #2214
Conversation
… on QueryRelationships and ReverseQueryRelationships iterators in the strict read mode proxy
github-actions
bot
added
area/datastore
tstirrat15
force-pushed
the
replicated-query
branch
from
4a28b29 to
656ffca
Compare
| replicaLoop: | ||
| for result, err := range it { | ||
| if err != nil { | ||
| // If the RevisionUnavailableError is returned on the first result, we should fallback | ||
| // to the primary. | ||
| if errors.As(err, &common.RevisionUnavailableError{}) { | ||
| if !isFirstResult { | ||
| yield(tuple.Relationship{}, spiceerrors.MustBugf("RevisionUnavailableError should only be returned on the first result")) | ||
| return | ||
| } | ||
| requiresFallback = true | ||
| break replicaLoop | ||
| } | ||
|
|
||
| if !yield(tuple.Relationship{}, err) { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| isFirstResult = false | ||
| if !yield(result, nil) { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Why was this additional layer necessary? Was this an observed failure mode?
There was a problem hiding this comment.
Yes; we need to catch it here as well
| if errors.As(err, &common.RevisionUnavailableError{}) { | ||
| log.Trace().Str("revision", rr.rev.String()).Msg("replica does not contain the requested revision, using primary") | ||
| return handler(rr.primary.SnapshotReader(rr.rev))(ctx, filter, options...) | ||
| } |
There was a problem hiding this comment.
I don't think this is needed here, as this error won't be bubbled up at this point because the iterator hasn't been returned yet: all we've done is return an iterator function.
There was a problem hiding this comment.
It could be if the underlying implementation changes; since that is not guaranteed, we still need to check for it here to future proof this
| replicated, err := NewStrictReplicatedDatastore(primary, replica) | ||
| require.NoError(t, err) | ||
|
|
||
| reader := replicated.SnapshotReader(revisionparsing.MustParseRevisionForTest("3")) |
There was a problem hiding this comment.
Please also test that a snapshot reader of ONLY the replica does return the error. Otherwise, this test could be silently failing (it does not - I traced it).
There was a problem hiding this comment.
Yes, I validated this test before I fixed the problem
josephschorr
force-pushed
the
replicated-query
branch
from
656ffca to
434547d
Compare
josephschorr
force-pushed
the
replicated-query
branch
from
434547d to
2bf945e
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
No description provided.