Skip to content
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

[Bugfix] Throw DocumentNotFoundError if index doesnt exist when looking for doc by id #2916

Merged

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented Oct 24, 2024

Changes

NotFoundError -> DocumentNotFoundError (consistent with the rest of framework)

The index refresh operation should be wrapped in a try/catch block. Otherwise, if we’re searching for a document in an index that doesn’t exist yet, it throws a NotFoundError instead of a DocumentNotFoundError. In my opinion, if the document doesn’t exist because the underlying index is missing, we should still return a DocumentNotFoundError. This also simplifies the caller logic, as we only need to handle a single exception type.

Update logs from critical to error. In my opinion, the critical log level should be reserved for cases that cause the entire framework to fail. In most, if not all, of these cases, we either retry or have some graceful handling, so it’s appropriate to log them at just the error level.

@jedrazb jedrazb changed the title [Bugfix] Gracefully handle non exisiting index when looking for doc by id [Bugfix] Throw DocumentNotFoundError if index doesnt exist when looking for doc by id Oct 24, 2024
@jedrazb jedrazb merged commit 18d2464 into main Oct 24, 2024
3 checks passed
@jedrazb jedrazb deleted the fix-agent-connector-bug-when-connector-index-doesnt-exist branch October 24, 2024 10:04
Copy link

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 2916 --autoMerge --autoMergeMethod squash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants