Fix sync parallel filters #468
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## stable #468 +/- ##
=======================================
Coverage 75.64% 75.64%
=======================================
Files 100 100
Lines 8769 8769
Branches 1714 1714
=======================================
Hits 6633 6633
Misses 1660 1660
Partials 476 476
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
infrahub_sdk/client.py
Outdated
| for page_number in range(1, total_pages + 1): | ||
| page_offset = (page_number - 1) * pagination_size | ||
| batch_process.add(task=process_page, node=node, page_offset=page_offset, page_number=page_number) | ||
| batch_process.add(task=process_page, page_offset=page_offset, page_number=page_number) |
There was a problem hiding this comment.
The removal of the node parameter here as well as in the async version is that something that's related to this change?
Looking at your change it doesn't seem like node provided any value previously. However we're still setting node with:
node = InfrahubNodeSync(client=self, schema=schema, branch=branch)I wonder if it makes sense to keep this, it's also a bit problematic as we're shadowing the node variable a bit further down in the method:
if populate_store:
for node in nodes:
if node.id:
self.store.set(node=node)
related_nodes = list(set(related_nodes))
for node in related_nodes:
if node.id:
self.store.set(node=node)There was a problem hiding this comment.
I could let node or do it in another PR, while debugging, I saw that we have node set twice out of 4 times when using the function, so from my point of view the function signature was not matching the usage.
There was a problem hiding this comment.
We can perhaps create a follow up issue to remove the node = parts in these two methods.
infrahub_sdk/client.py
Outdated
| for page_number in range(1, total_pages + 1): | ||
| page_offset = (page_number - 1) * pagination_size | ||
| batch_process.add(task=process_page, node=node, page_offset=page_offset, page_number=page_number) | ||
| batch_process.add(task=process_page, page_offset=page_offset, page_number=page_number) |
There was a problem hiding this comment.
We can perhaps create a follow up issue to remove the node = parts in these two methods.
Deploying infrahub-sdk-python with
|
| Latest commit: |
0dd2541
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://66a124a2.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://bkr-fix-sync-parallel-filter.infrahub-sdk-python.pages.dev |
When using the InfrahubClientSync.filters() with
parallel=True, the pagination was unstable, with some delta returned now and then.When dumping the queries, every time this issue happens, it seems like we are doing 2 times the 2nd page and not the first one. Forcing offset=0 fixes it.
I am not sure why this issue was not present with the Async Client, considering both of them are using the same
generate_query_data_init()...