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

util: Add WriteBatchInspector to inspect WriteBatch #1069

Merged
merged 8 commits into from
Nov 5, 2022

Conversation

mapleFU
Copy link
Member

@mapleFU mapleFU commented Nov 3, 2022

Hi, here I add a tool for inspecting WriteBatch, and using it for debugging

@mapleFU
Copy link
Member Author

mapleFU commented Nov 3, 2022

@PragmaTwice @git-hulk

I20221103 15:03:19.033007 40160 replication.cc:903] LogData(3 6)Put(�__namespace
�7Ok/d��AA?���������, 4373)PutCF(1, �__namespace
7Ok, �/d��AA?�������������������)
I20221103 15:03:19.033021 40160 replication.cc:903] LogData(3 6)Put(�__namespace
�7Ok/d��AA?���������, 4374)PutCF(1, �__namespace
7Ok, �/d��AA?�������������������)
I20221103 15:03:19.033035 40160 replication.cc:903] LogData(3 6)Put(�__namespace
�7Ok/d��AA?���������, 4375)PutCF(1, �__namespace
7Ok, �/d��AA?�������������������)
I20221103 15:03:19.033063 40160 replication.cc:903] LogData(3 6)Put(�__namespace
�7Ok/d��AA?���������, 4376)PutCF(1, �__namespace
7Ok, �/d��AA?�������������������)
I20221103 15:03:19.033077 40160 replication.cc:903] LogData(3 6)Put(�__namespace
�7Ok/d��AA?���������, 4377)PutCF(1, �__namespace
7Ok, �/d��AA?�������������������)

After inpecting hook, I think WriteBatch::Handler may works well, because program still coredump on InternalKey

@git-hulk
Copy link
Member

git-hulk commented Nov 3, 2022

cool, the batch looks correct, it may be caused by the uninitialized value. Can have a look at #1068

@mapleFU
Copy link
Member Author

mapleFU commented Nov 3, 2022

cool, the batch looks correct, it may be caused by the uninitialized value. Can have a look at #1068

Ok, the handler is pasted from rocksdb's TestHandler. It was used by rocksdb internal, so I need to copy it here. Should I just close the pull request or add WriteBatchInspector to our codebase?

@git-hulk
Copy link
Member

git-hulk commented Nov 3, 2022

@mapleFU I think we can keep this PR, coz it can make the batch debug easier.

@tisonkun tisonkun marked this pull request as draft November 4, 2022 01:47
@mapleFU
Copy link
Member Author

mapleFU commented Nov 4, 2022

@git-hulk update and making it as a tool

@mapleFU mapleFU changed the title [DNM] add batch-debugger and just testing Add WriteBatchInspector to inspect WriteBatch Nov 4, 2022
@mapleFU mapleFU changed the title Add WriteBatchInspector to inspect WriteBatch util: Add WriteBatchInspector to inspect WriteBatch Nov 4, 2022
@mapleFU mapleFU marked this pull request as ready for review November 4, 2022 02:20
@mapleFU
Copy link
Member Author

mapleFU commented Nov 4, 2022

@git-hulk I think it's ready to be merged now

@git-hulk
Copy link
Member

git-hulk commented Nov 4, 2022

@git-hulk I think it's ready to be merged now

OK, will take a look soon.

#include <string>
#include <vector>

/// Usage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we write some notes to explain why the file was introduced and what the motivation was. Otherwise other developers might wonder what the file was for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated now

git-hulk
git-hulk previously approved these changes Nov 4, 2022
@tanruixiang
Copy link
Member

I noticed that batch_debugger.h was migrated from Rocksdb, and I don't really know if there are other specifications when there are multiple Copyrights. Do you think the current copyright notice is reasonable? @tisonkun

@mapleFU
Copy link
Member Author

mapleFU commented Nov 4, 2022

I noticed that batch_debugger.h was migrated from Rocksdb, and I don't really know if there are other specifications when there are multiple Copyrights. Do you think the current copyright notice is reasonable?

A sample is https://github.com/facebook/rocksdb/blob/main/db/table_cache.h#L1-L8

PragmaTwice
PragmaTwice previously approved these changes Nov 4, 2022
caipengbo
caipengbo previously approved these changes Nov 5, 2022
@tisonkun
Copy link
Member

tisonkun commented Nov 5, 2022

if there are other specifications when there are multiple Copyrights

@tanruixiang in our NOTICE file there is an entry about RocksDB dependency, and RocksDB supports downstream use under the Apache License 2.0 - so basically, it won't hurt.

But we can improve the copyright info layout and add an original link here. @mapleFU if you'd like to, refer to:

https://github.com/apache/incubator-kvrocks/blob/77f37d5f88b205883de56196820a48ab1349d9d6/tests/tcl/tests/unit/expire.tcl#L21-L22

.. or I'll update it after this patch is merged. It's not a blocker, anyway.

@mapleFU mapleFU dismissed stale reviews from caipengbo, PragmaTwice, and git-hulk via aa74aa9 November 5, 2022 06:21
@mapleFU
Copy link
Member Author

mapleFU commented Nov 5, 2022

But we can improve the copyright info layout and add an original link here.

@tisonkun updated, PTAL

@tisonkun
Copy link
Member

tisonkun commented Nov 5, 2022

@mapleFU it seems that we don't use this Inspector in other code?

That said, if you'd like to debug, you add the code snippet as documented?

@mapleFU
Copy link
Member Author

mapleFU commented Nov 5, 2022

That said, if you'd like to debug, you add the code snippet as documented?

Yes, maybe user can receive raw bytes from remote and using this to parse the input @tisonkun

@tisonkun
Copy link
Member

tisonkun commented Nov 5, 2022

Merging...

@tisonkun tisonkun merged commit 19c6e95 into apache:unstable Nov 5, 2022
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.

6 participants