437 implement api to manage indexing queues #444
Conversation
src/db.py
Outdated
| for file in file_paths: | ||
| obj = QueuedFile( | ||
| ursadb_id=ursadb_id, | ||
| path=file.path, | ||
| index_types=file.index_types, | ||
| tags=file.tags, | ||
| ) | ||
| session.add(obj) | ||
| session.commit() |
There was a problem hiding this comment.
Ostatnio, gdy próbowałem robić inne zadanie i miałem do stworzenia wiele elementów, opłacało się skorzystać z bulk_insert_mappings zamiast dodawać iteracyjnie obiekt po obiekcie: https://stackoverflow.com/questions/34057006/using-bulk-insert-mappings, ewentualnie jakakolwiek inna operacja do wstawiania wielu modeli na raz.
Plus wydaje mi się, że obecnie po prostu iteracyjnie nadpisujesz tylko zmienną obj, po czym dodajesz i commitujesz tylko ostatnią instancję.
There was a problem hiding this comment.
o, faktycznie - jeszcze jedna tabulacja
| @app.delete( | ||
| "/api/query/{job_id}", | ||
| response_model=StatusSchema, | ||
| dependencies=[Depends(can_manage_queries)], |
src/db.py
Outdated
| alembic_cfg = Config(str(config_file)) | ||
| command.upgrade(alembic_cfg, "head") | ||
|
|
||
| def set_queued_file(self, ursadb_id, file_paths): |
There was a problem hiding this comment.
add_queued_file maybe, or enqueue_file
There was a problem hiding this comment.
(also ursadb: str, file_paths: list[str] guessing by variable names, but I didn't check the types)
There was a problem hiding this comment.
or List[FilePathInputSchema] actually I guess
src/schema.py
Outdated
|
|
||
| class FilePathInputSchema(BaseModel): | ||
| path: str | ||
| index_types: List[str] |
There was a problem hiding this comment.
We should validate the index_types somehow. I thought there's an enum for that, but apparently not. I think the easiest solution is to add a validator checking that every element is in ["gram3", "text4", "hash4", "wide8"]
There was a problem hiding this comment.
Can i use List[Literal["gram3", "text4", "hash4", "wide8"]] from typing?
msm-code
left a comment
msm-code
left a comment
There was a problem hiding this comment.
LGTM, I'll just finish testing it and merge
src/app.py
Outdated
| tags=["queue"], | ||
| dependencies=[Depends(can_manage_queues)], | ||
| ) | ||
| def delete_queued_by_ursadb_id(ursadb_id: str): |
There was a problem hiding this comment.
| def delete_queued_by_ursadb_id(ursadb_id: str): | |
| def delete_queue_by_id(ursadb_id: str): |
src/app.py
Outdated
| ursadb_exist = db.exist_ursadb(ursadb_id) | ||
| if ursadb_exist: | ||
| db.delete_queued_file(ursadb_id) | ||
| return StatusSchema(status="ok") | ||
|
|
||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Ursadb_id not found.", | ||
| ) |
There was a problem hiding this comment.
I think this method can just do nothing if there are no files in a given queue (i.e. no error)
src/app.py
Outdated
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Ursadb_id not found.", | ||
| ) |
There was a problem hiding this comment.
I don't think this should raise an exception if there are no files in the queue. Unfortunately, this means that oldest_file and newest_file must be nullable (they won't have a value if a queue is empty)
src/app.py
Outdated
| oldest_file = min(queue_files, key=lambda x: x.created_at) | ||
| newest_file = max(queue_files, key=lambda x: x.created_at) |
There was a problem hiding this comment.
If we don't use all the files, this should be done in the database (if there are many files this will be unnecessarily slow).
But we can discuss what's needed here
|
Thanks for fixing. I finally got around to re-testing it, sorry for the wait. Looks good (no need to mark it as a draft, in the future). |
msm-cert
left a comment
msm-cert
left a comment
There was a problem hiding this comment.
Looks really nice now, thanks
Your checklist for this pull request
What is the current behaviour?
We manually add files to the queue.
What is the new behaviour?
We have an API endpoint for adding files to QueuedFile in the database
Test plan
Endpoint api "api/queue/ursadb_id"
Closing issues
fixes #437