PIR: Add support for multiple profile queries in PirScan and PirOptOut #6413
Conversation
| }.flatten() | ||
| }.filter { it.value.isNotEmpty() } | ||
|
|
||
| val stepsCount = profileBrokerStepsMap.values.maxOf { it.size } |
There was a problem hiding this comment.
This logic seems incorrect. The thing that we want to do here is to use the total number of profile x broker x extractedProfile instances IF it’s lesser than the max limit. In this case, we are just taking the highest value.
| @@ -101,16 +104,22 @@ class RealBrokerStepsParser @Inject constructor( | |||
| override suspend fun parseStep( | |||
| brokerName: String, | |||
| stepsJson: String, | |||
| profileQuery: ProfileQuery?, | |||
There was a problem hiding this comment.
NIT: We can just pass the profileQuery.Id here too.
| }.filter { it.value.isNotEmpty() } | ||
|
|
||
| // Start each runner on the profile with the broker steps | ||
| profileBrokerStepsMap.entries.map { (profileQuery, brokerSteps) -> |
There was a problem hiding this comment.
Why entries.map and not forEach?
| } | ||
| }.awaitAll() | ||
| // Start each runner on the profile with the broker steps | ||
| profileBrokerStepsMap.entries.map { (profileQuery, brokerSteps) -> |
There was a problem hiding this comment.
Same issue here, we are not utilizing the maximum limit of webviews. Consider if we have this map:
mapOf{
profileA, 4 extracted profiles
profileB, 1 extracted profiles
profileC, 15 extracted profiles
)
It will only use 4 webviews initially and then 1 and then 15. Ideally this could all have run 20 webviews all at the same time.
There was a problem hiding this comment.
As discussed, there is a limitation to the runners that requires a profile query to be passed in along with the steps, which makes it a bit harder to implement an optimized solution. I will take some time (until Monday) to try to implement a more optimized solution that utilizes all available runners.
| // Start each runner on a subset of the broker steps. | ||
| brokers.mapNotNull { broker -> | ||
| // Split all the broker steps into parts that can be executed in parallel | ||
| val brokerScanSteps = brokers.mapNotNull { broker -> |
There was a problem hiding this comment.
This works but could’ve be improved if we consider how many profile x broker combinations there would be and then divide that according to the limit.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1210594645151737/task/1210750227652612?focus=true
Description
Adds support for running the initial PIR scan and PIR opt-out scan on multiple stored profiles instead of the first stored profile. See the task for more details and a full tech spec.
Steps to test this PR
1 profile (current behavior)
Multiple profiles (new behavior)
UI changes
No UI Changes