kvrocks Avoid using Get interface to obtain metadata when iterating data during migration

Fixed #904

Sep 22 '22 07:09 caipengbo

The problem we had was that when we migrated, we had a lot of disk reads, but with this optimized version, these abnormal disk reads disappeared, so I think GetMetaData calls the Get interface

Sep 22 '22 07:09 datavisorxiaobiaozhao

The problem we had was that when we migrated, we had a lot of disk reads, but with this optimized version, these abnormal disk reads disappeared, so I think GetMetaData calls the Get interface.

@datavisorxiaobiaozhao Yes. During the iteration, we set the cache not to be filled:

https://github.com/apache/incubator-kvrocks/blob/e8721a270d23345f385af6db0617c3549d6e6812/src/slot_migrate.cc#L273-L277

so that when we call DB::Get(), we are likely to read the disk.

Sep 22 '22 08:09 caipengbo

There may be some problem. The Get interface may aim to get the latest expire time of the key.

Sep 23 '22 06:09 shangxiaoxiong

There may be some problem. The Get interface may aim to get the latest expire time of the key.

@shangxiaoxiong Oh Yes, I missed that. Then we only need GetMetaData for those with expiration time. Or we can record the time snapshot was created and subtract the expiration time to determine whether it is expired!

Sep 23 '22 06:09 caipengbo

During sending snapshot, exipre time of some key may be changed to larger by clients. GetMetaData is needed to deal with this case.

Sep 23 '22 07:09 shangxiaoxiong

l think GetMetaData is needed for complex key with expire time to get the latest expire time.

Sep 23 '22 07:09 shangxiaoxiong

During sending snapshot, exipre time of some key may be changed to larger by clients. GetMetaData is needed to deal with this case.

During sending snapshot:

for simple type, user changes to expire are incremental data and will be synchronized through WAL.
for complex types, we really should get the latest metadata.

Sep 23 '22 07:09 caipengbo

During sending snapshot, exipre time of some key may be changed to larger by clients. GetMetaData is needed to deal with this case.

During sending snapshot, user changes to expire are incremental data and will be synchronized through WAL.

Simple key is ok but complex key is not.

Sep 23 '22 07:09 shangxiaoxiong

During sending snapshot, exipre time of some key may be changed to larger by clients. GetMetaData is needed to deal with this case.

During sending snapshot, user changes to expire are incremental data and will be synchronized through WAL.

Simple key is ok but complex key is not.

It can't resolve this issue even we use the latest snapshot since the change may come after.

Sep 23 '22 08:09 git-hulk

Yeah. Using GetMetaData with latest snapshot can't solve the problem completely. Just a little improvement.

Sep 23 '22 08:09 shangxiaoxiong

Indeed, the whole process is not atomic.

Sep 23 '22 08:09 caipengbo

Yes, the root cause is the snapshot can be different between writers and background threads. I prefer to use current implementation, then we can try to seek another way to resolve this issue completely. How do you think? @caipengbo @shangxiaoxiong

Sep 23 '22 08:09 git-hulk

Current implementation makes slotmigration faster. Faster slotmigration is, smaller risk of losing key is. On the whole, current implementation may be good.

Sep 23 '22 08:09 shangxiaoxiong

I think OK, I will continue to think about how to solve this kind of problem.

Sep 23 '22 09:09 caipengbo

I think this is the best solution for now, merge this pr first, and then contionue to look for new solutions.

Sep 23 '22 12:09 xiaobiaozhao

Thanks all for the great discussion, I'll explain the problems with this implementation in #904

Sep 23 '22 12:09 caipengbo