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

Take to account all new partitions when checking shard limits (backport #15843) #15852

Merged
merged 1 commit into from Apr 17, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Apr 17, 2024

Takes a fix from #15813 but with a simpler test.

It's possible to significantly exceed the limit: values_count/nodes times.
If initial setup was fine for a single partition, the whole insert passed as we took to account only partition.

This is not a regression caused by #13843.
I also checked in 5.2, same problem there. In 5.2 we used to create "probe" index multiple times via indicesService.createIndex but it doesn't affect
int currentOpenShards = state.metadata().getTotalOpenIndexShards()
(probably because we call allocationService.reroute only once after the loop)

This is not a fix for #15803 (which is about expanding replicas, or something else but not related to partitioned tables. Maybe even expected and need to expand docs. I will have a dedicated PR for it, out of the scope of this PR).

insert from select is also slightly affected but I couldn't significantly exceed the limit.
BulkShardCreationLimiter leads to creating new partitions in smaller batches, so creating a batch actually increases
state.metadata().getTotalOpenIndexShards() and such ShardLimitValidator kinda keeps up with the actual state and cannot significantly exceed the limit. In other words, it doesn't take to account not total_new_partitions - 1 but "only" new_partitions_batch_size - 1

Anyway, this change also addresses insert from select as well, even though it's less visible problem that insert-from-values


This is an automatic backport of pull request #15843 done by [Mergify](https://mergify.com).

@crate-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@BaurzhanSakhariev
Copy link
Contributor

ok to test

@BaurzhanSakhariev BaurzhanSakhariev added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Apr 17, 2024
@mergify mergify bot merged commit 4f389d2 into 5.7 Apr 17, 2024
14 checks passed
@mergify mergify bot deleted the mergify/bp/5.7/pr-15843 branch April 17, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Let Mergify merge the PR once approved and checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants