-
Notifications
You must be signed in to change notification settings - Fork 27
Clear scroll id context #419
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
Conversation
biothings/web/query/engine.py
Outdated
| await self.client.clear_scroll(scroll_id=query.data) | ||
| logger.info("Scroll context cleared: %s", scroll_id) | ||
| except Exception as e: | ||
| logger.warning("Failed to clear scroll context (ID: %s): %s", scroll_id, str(e)) |
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.
What's an example where we wouldn't be able to clear the scroll context? Would it be a networking issue or like if something interrupted the context?
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.
Yes, that could be errors. It could also happen if the context was already cleaned up.
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.
When it tries to clear a non-existent scroll_id, it returns a NotFoundError 404.
biothings/web/query/engine.py
Outdated
| raise RawResultInterrupt(res) | ||
|
|
||
| if not res["hits"]["hits"]: | ||
| scroll_id = str(query.data) if query.data else "None" |
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.
do we need this here? if isinstance(query, ESScrollID) is True, is there a case query.data is not a scroll_id?
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.
You are right, we don't need this here.
biothings/web/query/engine.py
Outdated
| if not res["hits"]["hits"]: | ||
| scroll_id = str(query.data) if query.data else "None" | ||
| try: | ||
| await self.client.clear_scroll(scroll_id=query.data) |
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.
if you already have a scroll_id variable before this line, should we just pass scroll_id here?
No description provided.