-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[BUG]: WAL replay warnings suppression #1763
[BUG]: WAL replay warnings suppression #1763
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
fa94bdb
to
71e7619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is deeply suspicious to me, and there isn't sufficient detail in the description for me to understand it. Unless I've misunderstood, we should never be emitting duplicate ID warnings - I suspect this might be a real bug in the WAL logic.
@atroyn, I've added a colab that reproduces the problem. The gist is that we emit all entries from the WAL up to the index' MAX seqId, if there are duplicate entries (e.g. an item was added/removed multiple times) the respective index reports the duplicates as warnings. |
Similar issue was reported by MemGPT - https://discord.com/channels/1073293645303795742/1205288940488359998 |
@@ -288,7 +288,9 @@ def _backfill(self, subscription: Subscription) -> None: | |||
.where(t.topic == ParameterValue(subscription.topic_name)) | |||
.where(t.seq_id > ParameterValue(subscription.start)) | |||
.where(t.seq_id <= ParameterValue(subscription.end)) | |||
.select(t.seq_id, t.operation, t.id, t.vector, t.encoding, t.metadata) | |||
.select( | |||
t.seq_id, t.operation, t.id, t.vector, t.encoding, t.metadata, t.topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think topic
is needed here. Perhaps wrongfully added during testing/experimentation
This is potentially made redundant by #2062 |
Close in favor of #2062 |
Refs: #1733
Description of changes
Summarize the changes made by this PR.
EmbeddingRecord
flag (wal_replay
) to suppress warnings in sqlite and local hnsw indicesTest plan
How are these changes tested?
pytest
for python,yarn test
for jsDocumentation Changes
N/A
Colab to reproduce the issue in 0.4.23 (and prior) - https://colab.research.google.com/drive/1CtN0qGoMZoVfwQBnJE1MloLvvdv5KouE?usp=sharing