Add Octopoes bulk reports API #4219
Conversation
|
| report_type="concatenated-report", | ||
| ) | ||
| octopoes_api_connector.save_declaration(Declaration(ooi=recipe, valid_time=valid_time)) | ||
| octopoes_api_connector.save_declaration(Declaration(ooi=report, valid_time=valid_time)) |
|
|
||
| if not ignore_count: | ||
| count_results = self.query(query.count(), valid_time) | ||
| count = 0 if not count_results else count_results[0] # type: ignore |
There was a problem hiding this comment.
Why should this type error be ignored? What kind of type error are you getting?
| """ | ||
| res = self.session.post("/reports", json=reports_filters, params={"valid_time": str(valid_time)}) | ||
|
|
||
| return TypeAdapter(dict[UUID, HydratedReport]).validate_json(res.content) |
There was a problem hiding this comment.
I suggest extracting this TypeAdapter instance to a variable outside the class, since they're quite costly to make. This way the TypeAdapter can be reused
| reports = {} | ||
|
|
||
| for client, recipe_id in reports_filters: | ||
| xtdb_http_client._client = client |
There was a problem hiding this comment.
Accessing a private attribute like this is not Pythonic. Perhaps this attribute can now be a public property instead?
IMO client here is a bit ambiguous. Maybe not for now, but I suggest eventually renaming either the attribute or variable name to organization_code
| The reason for creating the event_manager do this in the loop is the '_try_connect()' call in the __init__ possibly | ||
| slowing this down, while this API was introduced to improve performance. Simply reusing it for all clients works | ||
| because the event manager is only used in callbacks triggered on a `commit()`, while these queries are read-only and | ||
| hence don't need a `commit()` as no events would be triggered. (A cleaner solution would perhaps be to extract an | ||
| interface and pass a new NullManager.) | ||
|
|
||
| The xtdb_http_client is also created outside the loop and the `_client` property changed inside the loop instead, | ||
| to reuse the httpx Session for all requests. |
There was a problem hiding this comment.
This is useful information, but I recommend placing it elsewhere in the method rather than in the docstring. Since the docstring serves as API documentation, this information isn't relevant to API users
Changes
Another performance enhancement for #4007
Issue link
Demo
QA notes
There is no real way to QA the new endpoint now unless we start talking to the Octopoes API directly. However, this will be QA'd once integrated in #4007.
Code Checklist
Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.