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.

Sign up for GitHub

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

Jump to bottom

Adding beezcli #427

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Adding beezcli #427

merged 1 commit into from
Apr 5, 2023

Conversation

ofriedma
Copy link
Contributor

@ofriedma ofriedma commented Mar 15, 2023 •
edited
Loading

This Pull request is adding a wrapper around ldb and allow using ldb in interactive manner (it is yet possible to use it non interactive just like ldb).
It adds some new functionalities to ldb such as multiget,
summary of keys and values from dump command, allow to not present values on dump and idump.
Fix the ttl parameter, to allow ttl to get value and run compaction on top of db with ttl.

Resolves: #407

@ofriedma ofriedma added enhancement New feature or request usability To review A feature request that has not been reviewed yet bug fix Fixes a known bug labels Mar 15, 2023
@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch from a469d0d to 335aca0 Compare March 15, 2023 15:32
@ofriedma
Copy link
Contributor Author

ofriedma commented Mar 15, 2023 •
edited
Loading

It is possible to review it

@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch from 335aca0 to bf8e845 Compare March 16, 2023 13:50
@ofriedma ofriedma requested a review from ayulas March 20, 2023 14:09
ayulas
ayulas reviewed Mar 20, 2023
View reviewed changes
examples/speedb_with_ttl_example.cc Outdated
<< std::endl
<< get_value << std::endl
<< std::endl;
std::cout << "The value returned by db MultiGet before expiration are: "
Copy link
Contributor

Choose a reason for hiding this comment

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

pls move it as you did in the Get to after the call MultiGet

examples/speedb_with_ttl_example.cc Outdated

s = db->Get(ropts, key1, &get_value);
if (s.IsNotFound()) {
std::cout << "Key has been expired as expected" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

add by Get to the output

examples/speedb_with_ttl_example.cc Outdated
}
statuses = db->MultiGet(ropts, keys, &values);
for (const auto& i : statuses) {
if (s.IsNotFound()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s??? the for is i...

your test passed cause you used previous s value from the Get...

examples/speedb_with_ttl_example.cc Outdated
}
}
ropts.skip_expired_data = false;
std::cout << "Keys actually stored but expired by MultiGet, without "
Copy link
Contributor

Choose a reason for hiding this comment

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

be consistent - the output after the call as mention above

examples/speedb_with_ttl_example.cc Outdated
statuses = db->MultiGet(ropts, keys, &values);
for (size_t i = 0; i < statuses.size(); ++i) {
if (statuses[i].ok()) {
std::cout << keys[i].ToStringView() << ":" << values[i] << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

dont you want to verify it is the values you are expected too?

utilities/ttl/ttl_test.cc Outdated
ropts.skip_expired_data = true;
std::vector<std::string> values;
ASSERT_TRUE(db_ttl_->MultiGet(ropts, {"a"}, &values)[0].IsNotFound());
ASSERT_TRUE(values[0].empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

why you need it... if the MultiGet return not found ofcourse no values...

utilities/ttl/ttl_test.cc Outdated
CloseTtl();
}

TEST_F(TtlTest, DoNotSkipUnExpiredTtlGetTest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do not skip??? you set skip.. . you get the value cause it wasnt expired

utilities/ttl/ttl_test.cc Outdated
CloseTtl();
}

TEST_F(TtlTest, DoNotSkipReadOnlyTtlMultiGetTest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

again the name do not skip is not what you set... ropts.skip_expired_data = true;

utilities/ttl/ttl_test.cc Outdated
CloseTtl();
}

TEST_F(TtlTest, DoNotSkipReadOnlyTtlGetTest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

utilities/ttl/ttl_test.cc Outdated
h = nullptr;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to add test that do flush/compaction after ttl expired so you see that eventhough you set skip_expired_data=false after flush if you try get the keys were skipped in the flush and werent written to the sst so they wont exist

@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch 4 times, most recently from e5c6fe6 to 840e249 Compare March 21, 2023 15:50
ofriedma
ofriedma commented Mar 21, 2023
View reviewed changes
tools/ldb_cmd.cc Outdated
@@ -500,7 +516,7 @@ void LDBCommand::OpenDB() {
}
} else {
// We successfully opened DB in single column family mode.
assert(column_families_.empty());
// assert(column_families_.empty());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what to do with this assert

ayulas
ayulas previously approved these changes Mar 21, 2023
View reviewed changes
@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch 3 times, most recently from 8069dbd to 103f9de Compare April 3, 2023 15:47
@ofriedma ofriedma requested a review from ayulas April 3, 2023 15:47
@ofriedma
Copy link
Contributor Author

ofriedma commented Apr 3, 2023

@ayulas i added unit test and more comments other than that the code is the same has reviewed

ayulas
ayulas previously approved these changes Apr 4, 2023
View reviewed changes
@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch from 103f9de to 384f7f4 Compare April 4, 2023 13:25
@ofriedma ofriedma requested a review from ayulas April 4, 2023 13:43
ayulas
ayulas previously approved these changes Apr 4, 2023
View reviewed changes
@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch from 384f7f4 to 941dfe2 Compare April 4, 2023 15:28
@ofriedma ofriedma requested a review from ayulas April 4, 2023 15:30
@ofriedma
Copy link
Contributor Author

ofriedma commented Apr 4, 2023 •
edited
Loading

@ayulas added #include <stdio.h> to beezcli.cc because it had an issue when compiled with clang

ayulas
ayulas previously approved these changes Apr 4, 2023
View reviewed changes
adding beezcli and ttl support speedb-io#407
c37ed78 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: speedb-io#407
@ofriedma ofriedma force-pushed the wip-ofriedma-beezcli branch from 941dfe2 to c37ed78 Compare April 4, 2023 15:57
@ofriedma ofriedma requested a review from ayulas April 4, 2023 16:27
@ofriedma
Copy link
Contributor Author

ofriedma commented Apr 4, 2023

added //clang-format off and //clang-format on around #include <stdio.h> otherwise clang-format will try to put stdio after readline which will lead to compilation error in clang

@ofriedma ofriedma requested a review from Guyme April 4, 2023 16:33
@Yuval-Ariel Yuval-Ariel merged commit 4091672 into speedb-io:main Apr 5, 2023
Yuval-Ariel pushed a commit that referenced this pull request May 2, 2023
@ofriedma @Yuval-Ariel
adding beezcli and ttl support (#427)
d487146 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
Yuval-Ariel pushed a commit that referenced this pull request May 4, 2023
@ofriedma @Yuval-Ariel
adding beezcli and ttl support (#427)
57e540c 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
udi-speedb pushed a commit that referenced this pull request Nov 13, 2023
@ofriedma @udi-speedb
adding beezcli and ttl support (#427)
c9e4c41 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
udi-speedb pushed a commit that referenced this pull request Nov 15, 2023
@ofriedma @udi-speedb
adding beezcli and ttl support (#427)
c7bab4f 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
udi-speedb pushed a commit that referenced this pull request Dec 4, 2023
@ofriedma @udi-speedb
adding beezcli and ttl support (#427)
ed446ef 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
ofriedma added a commit that referenced this pull request Dec 5, 2023
@ofriedma
adding beezcli and ttl support (#427)
cb12a68 
beezcli is an interactive tool wrapping ldb

This commit adds some enhancement to ldb

Resolves: #407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guyme Guyme Guyme approved these changes

@ayulas ayulas Awaiting requested review from ayulas

Assignees

@Guyme Guyme

Labels
bug fix Fixes a known bug enhancement New feature or request To review A feature request that has not been reviewed yet usability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Speedb_CLI - a query and maintance tool for off-line Speedb databases
4 participants
@ofriedma @ayulas @Guyme @Yuval-Ariel