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

engine: fix prefix extractor panic when delete range #4503

Merged
merged 5 commits into from Apr 10, 2019

Conversation

Projects
None yet
6 participants
@overvenus
Copy link
Member

overvenus commented Apr 10, 2019

What have you changed? (mandatory)

Delete range as a kv pair is passed to prefix extractor which causes panic, because the key is less than 8 bytes.

This is a temporary workaround, the long term plan is skipping the prefix extractor for delete range in RocksDB side.

What are the type of the changes? (mandatory)

  • Bug fix (change which fixes an issue)

How has this PR been tested? (mandatory)

Unit tests.

Does this PR affect documentation (docs) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

No.

Refer to a related PR or issue link (optional)

#4461

engine: fix prefix extractor panic when delete range
Signed-off-by: Neil Shen <overvenus@gmail.com>
more tests
Signed-off-by: Neil Shen <overvenus@gmail.com>
@zhangjinpeng1987
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Apr 10, 2019

@yiwu-arbug Can RangeDeletion be skipped when we set prefix bloom filter?

Address comment
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Apr 10, 2019

/run-integration-tests

Address comments
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Apr 10, 2019

/run-unit-test

@kennytm kennytm added the S: LGT1 label Apr 10, 2019

@zhouqiang-cl

This comment has been minimized.

Copy link
Contributor

zhouqiang-cl commented Apr 10, 2019

/run-unit-test

1 similar comment
@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Apr 10, 2019

/run-unit-test

@zhangjinpeng1987
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

@zhangjinpeng1987 zhangjinpeng1987 merged commit 57ce5ab into tikv:master Apr 10, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@overvenus overvenus deleted the overvenus:delete-range-write branch Apr 10, 2019

@yiwu-arbug

This comment has been minimized.

Copy link
Contributor

yiwu-arbug commented Apr 10, 2019

@overvenus Do you have a stack trace of the panic, or can you show me how can I reproduce? I tried a simple test with RocksDB and it works fine: yiwu-arbug/rocksdb@d72564c

@yiwu-arbug

This comment has been minimized.

Copy link
Contributor

yiwu-arbug commented Apr 12, 2019

Fixing prefix extractor issue on RocksDB side with facebook/rocksdb#5190 and facebook/rocksdb#5191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.