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

feat: Yield inside huge values migration serialization #4197

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Nov 26, 2024

With #4144 we break huge values slot migration into multiple commands. This PR now adds yield between those commands.
It also adds a test that checks that modifying huge values while doing a migration works well, and that RSS doesn't grow too much.

Fixes #4100

With #4144 we break huge values slot migration into multiple commands.
This PR now adds yield between those commands.
It also adds a test that checks that modifying huge values while doing a
migration works well, and that RSS doesn't grow too much.

Fixes #4100
@chakaz
Copy link
Collaborator Author

chakaz commented Nov 26, 2024

@adiholden re/ locks during serialization:

DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

@kostasrim
Copy link
Contributor

@adiholden re/ locks during serialization:

DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

don't worry about local_mutex it's removed in my PR (assuming by local_mutex you mean the db_slice member)

@chakaz
Copy link
Collaborator Author

chakaz commented Nov 26, 2024

@adiholden re/ locks during serialization:
DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

don't worry about local_mutex it's removed in my PR (assuming by local_mutex you mean the db_slice member)

I do refer to DbSlice's local_mu_, and I actually do need to lock it for the same reason PreUpdate locks it today.

@chakaz chakaz requested a review from adiholden November 28, 2024 10:50
if seed_during_migration:
await stop_seed()
else:
# Only verify memory growth if we haven't pushed new data during migration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we need this option of not running seeder during migration
you added this option inorder to check the rss compared to rss before migration right? but we can compare peak rss to peak used memory


insert_task = asyncio.create_task(insert_data(instances[0].cluster_client()))

async def get_rss(client, field):
Copy link
Collaborator

Choose a reason for hiding this comment

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

get_meomory_info maybe?

while True:
rss = await get_rss(nodes[0].client, "used_memory_rss")
logging.debug(f"Current rss: {rss}")
if rss > 1_000_000_000:
Copy link
Collaborator

Choose a reason for hiding this comment

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

explain why are you waiting for 1G rss?

# Insert data to containers with a gaussian distribution: some will be small and other big
stop = False

async def insert_data(client):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kostas is working on seeder improvement for containers with different size
I prefer to have something generic and this logic inside test

@chakaz
Copy link
Collaborator Author

chakaz commented Dec 8, 2024

Please do not review yet. I still haven't modified the tests to use Kostas' new framework. This is just a merge of changes from main.

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.

Big value serialization in migration
3 participants