Skip to content
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

Assign all sources/shards, even if this require exceeding the indexer #4363

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

fulmicoton
Copy link
Contributor

capacities.

We do that simply by scaling the indexer capacities. First we scale the indexer capacities so that the sum of their capacities exceeds the total load of the source to assign.

If the packing algorithm fails, we inflates the capacities again by 10% and retry.

Closes #4290

@fulmicoton fulmicoton marked this pull request as ready for review January 10, 2024 09:43
@fulmicoton fulmicoton force-pushed the issue/4290-allow-over-assigning branch 4 times, most recently from ecc24a0 to 005380e Compare January 10, 2024 09:47
@fulmicoton fulmicoton requested a review from fmassot January 11, 2024 01:50
}
}
}
assert!(load_in_node <= indexer_max_loads.get(node_id).unwrap().cpu_millis());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this issue, this assert is actually invalid so I removed it.
I added an assert checking that the physical plan matches the solution.

@fulmicoton fulmicoton force-pushed the issue/4290-allow-over-assigning branch 7 times, most recently from 8e00514 to d213de3 Compare January 12, 2024 03:15
capacities.

We do that simply by scaling the indexer capacities.
First we scale the indexer capacities so that the sum of their
capacities exceeds the total load of the source to assign.

If the packing algorithm fails, we inflates the capacities again
by 10% and retry.

Added assert on convert solution to physical solution.

Closes #4290
@fulmicoton fulmicoton force-pushed the issue/4290-allow-over-assigning branch from d213de3 to 0374250 Compare January 12, 2024 09:57
@@ -124,7 +125,21 @@ fn convert_physical_plan_to_solution(
source_id: indexing_task.source_id.clone(),
};
if let Some(source_ord) = id_to_ord_map.source_ord(&source_uid) {
indexer_assignment.add_shards(source_ord, indexing_task.shard_ids.len() as u32);
let source_type = &source_ord_map.get(&source_ord).unwrap().source_type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always afraid of those unwraps, I'm unsure if a comment is worth it on the function or maybe replace unwrap by expect("")

// we are certain that even on our first attempt, the total capacity of the indexer exceeds 120%
// of the partial solution.
//
// 1.2^30 > 240.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.2**30 ~ 237.37

Copy link
Contributor

@fmassot fmassot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Do you know if our tests are currently sufficient? Wondering if adding protesting is worth it

@fulmicoton fulmicoton merged commit 2c21350 into main Jan 15, 2024
2 checks passed
@fulmicoton fulmicoton deleted the issue/4290-allow-over-assigning branch January 15, 2024 02:25
@fulmicoton
Copy link
Contributor Author

fulmicoton commented Jan 15, 2024

So we have 2 layers here: we rewrite the problem in a space where all shards have the same type, all source have the same type, and source and indexer are identified by a small indexable id.

So the logic goes

  • take actual problem
  • transform it into logical problem
  • solve logical problem
  • transform logical solution into physical solution

The logical part is proptested already.
We do not proptest conversion from physical to logical stuff and we have a bunch of very defensive assert checking postconditions everywhere.

So far the bugs have been related to edge case on the conversion stuff.
We could add proptesting there too, but I think traditional unit testing is ok there. (It is not prone to weird effect)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow over assigning
2 participants