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

Avoid using Get interface to obtain Metadata when iterating data during migration #904

Closed
caipengbo opened this issue Sep 22, 2022 · 4 comments · Fixed by #906
Closed

Avoid using Get interface to obtain Metadata when iterating data during migration #904

caipengbo opened this issue Sep 22, 2022 · 4 comments · Fixed by #906
Labels
enhancement type enhancement

Comments

@caipengbo
Copy link
Contributor

caipengbo commented Sep 22, 2022

Motivation

When migrating data, I found that when migrating the same number of keys (about 10W), the migration time was very variable, sometimes very short, sometimes very long. When the migration took a long time, I found that the CPU was full.

Looking at the flame diagram below:

image

we can see that there is a deep call stack on the left.

Zoomed in it:

image

When we parsed and sent the full data (SlotMigrate::SendSnapshot()), we used the rocksdb::DB::Get() interface to get the metadata, which was CPU consuming.

How much does this affect migration speed?

  1. Because we set the iterator to not fill the cache, when the amount of data is larger than cache, it is very likely that we will call rocksdb::DB::Get() to read disk, which greatly affects the performance!
  2. If the retrieval in memtable is bad( it depends on workload), then it will consume more CPU, which will bring more serious impact.

Solution

In fact, we do not need to get the value through the rocksdb::DB::Get() interface. When we iterate data, value and key are together, so we can directly get the value through the iterator and then encode into metadata.

@caipengbo caipengbo added the enhancement type enhancement label Sep 22, 2022
@git-hulk
Copy link
Member

Yes, good catch.

@datavisorxiaobiaozhao
Copy link

I have tested and confirmed that this is an issue. The fix works and can speed up migration by 10-100 times.Looking forward to your PR

@shangxiaoxiong
Copy link
Member

I have tested and confirmed that this is an issue. The fix works and can speed up migration by 10-100 times.Looking forward to your PR

More than 10 times? Are you serious?

@caipengbo
Copy link
Contributor Author

The original purpose of using rocksdb::DB::Get() to Get Metadata, I guess is to get the latest metadata (but mistakenly using the old snapshot).

The purpose of this is: snapshot is created at the beginning of the migration. If a slot has been migrated for a long time, users may change the number of subkeys in complex types or change the expiration time. If we use metadata in very old snapshot to determine the expiration time, we may make a mistake. The subkey cannot be migrated to the target end. WAL only records metadata changes, not subkeys. In this case, subkeys will be completely lost.

However, as we discussed in #906, even if we get the latest snaphot, we still can't prevent users from modifying complex types of metadata. We still make errors in judgment.

As of now, this problem is unavoidable, and we welcome discussion of effective solutions.

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

Successfully merging a pull request may close this issue.

4 participants