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

feat: support limit WriteBatch size #2508

Merged
merged 9 commits into from
Sep 4, 2024
Merged

feat: support limit WriteBatch size #2508

merged 9 commits into from
Sep 4, 2024

Conversation

AntiTopQuark
Copy link
Contributor

relate to #2284

Added the ability to limit the size of WriteBatch operations in RocksDB to control memory usage effectively.

git-hulk
git-hulk reviewed Aug 28, 2024
View reviewed changes
src/config/config.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@AntiTopQuark Thanks for your efforts. Would you mind adding a Go test case for this?

@AntiTopQuark
Copy link
Contributor Author

@AntiTopQuark Thanks for your efforts. Would you mind adding a Go test case for this?

Sure, I will complete this task later.

@AntiTopQuark AntiTopQuark force-pushed the limit_write_batch branch 2 times, most recently from ba0b85d to d19064e Compare August 28, 2024 15:54
@AntiTopQuark
Copy link
Contributor Author

While adding cases, it was found that there were no corresponding error messages, so error codes were added and reported in many places involving batch processing

@git-hulk
Copy link
Member

While adding cases, it was found that there were no corresponding error messages, so error codes were added and reported in many places involving batch processing

Yes, that makes sense.

git-hulk
git-hulk reviewed Aug 29, 2024
View reviewed changes
src/search/indexer.cc Outdated Show resolved Hide resolved
src/search/indexer.cc Outdated Show resolved Hide resolved
src/types/redis_stream.cc Outdated Show resolved Hide resolved
src/types/redis_zset.cc Outdated Show resolved Hide resolved
src/types/redis_zset.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@AntiTopQuark One comment inline, rest are good to me. Could anyone also have a look at this PR? @PragmaTwice @torwig @caipengbo @mapleFU

caipengbo
caipengbo previously approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@caipengbo caipengbo left a comment

Choose a reason for hiding this comment

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

LGTM

kvrocks.conf Outdated Show resolved Hide resolved
caipengbo
caipengbo previously approved these changes Aug 31, 2024
View reviewed changes
caipengbo
caipengbo previously approved these changes Sep 2, 2024
View reviewed changes
git-hulk
git-hulk previously approved these changes Sep 3, 2024
View reviewed changes
mapleFU
mapleFU reviewed Sep 3, 2024
View reviewed changes
src/search/hnsw_indexer.cc Outdated
Comment on lines 89 to 93
auto s = PutMetadata(&node_metadata, search_key, ctx.storage, batch);
if (!s.IsOK()) {
return s;
}
return Status::OK();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto s = PutMetadata(&node_metadata, search_key, ctx.storage, batch);
if (!s.IsOK()) {
return s;
}
return Status::OK();
return PutMetadata(&node_metadata, search_key, ctx.storage, batch);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mapleFU
mapleFU reviewed Sep 3, 2024
View reviewed changes
src/search/hnsw_indexer.cc Outdated
@@ -93,7 +103,10 @@ Status HnswNode::RemoveNeighbour(engine::Context& ctx, const NodeKey& neighbour_

HnswNodeFieldMetadata node_metadata = GET_OR_RET(DecodeMetadata(ctx, search_key));
node_metadata.num_neighbours--;
PutMetadata(&node_metadata, search_key, ctx.storage, batch);
auto redis_status = PutMetadata(&node_metadata, search_key, ctx.storage, batch);
Copy link
Member

Choose a reason for hiding this comment

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

RETURN_NOT_OK or just return this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mapleFU
mapleFU requested changes Sep 3, 2024
View reviewed changes
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

This pr LGTM. But it can be shorter with macros

@AntiTopQuark AntiTopQuark dismissed stale reviews from git-hulk and caipengbo via 216567b September 3, 2024 07:14
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 3, 2024

Quality Gate Passed Quality Gate passed

Issues
42 New issues
0 Accepted issues

Measures
0 Security Hotspots
56.7% Coverage on New Code
4.4% Duplication on New Code

See analysis details on SonarCloud

mapleFU
mapleFU approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Also cc @PragmaTwice

Do we need some macro like RETURN_NOT_OK, RETURN_NOT_OK_FROM_ROCKSDB?

@mapleFU mapleFU merged commit c75ad64 into apache:unstable Sep 4, 2024
32 checks passed
@git-hulk
Copy link
Member

git-hulk commented Sep 4, 2024

Also cc @PragmaTwice

Do we need some macro like RETURN_NOT_OK, RETURN_NOT_OK_FROM_ROCKSDB?

@mapleFU @AntiTopQuark RETURN_IF_ERR is also good for this?

@mapleFU
Copy link
Member

mapleFU commented Sep 4, 2024

I think it's good. Another problem is that we may need type cast between Status and rocksdb::Status. We can make a new issue here.

@git-hulk
Copy link
Member

git-hulk commented Sep 4, 2024

Yes, let's discuss this in new issue.

@git-hulk git-hulk mentioned this pull request Sep 4, 2024
Open
@PragmaTwice
Copy link
Member

Also cc @PragmaTwice

Do we need some macro like RETURN_NOT_OK, RETURN_NOT_OK_FROM_ROCKSDB?

Nope. They can be done by GET_OR_RET.

@mapleFU
Copy link
Member

mapleFU commented Sep 4, 2024

Nope. They can be done by GET_OR_RET.

So GET_OR_RET can also handle the case for pure "Status"?

Besides, what about err from rocksdb?

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

@mapleFU mapleFU mapleFU approved these changes

@torwig torwig Awaiting requested review from torwig

@PragmaTwice PragmaTwice Awaiting requested review from PragmaTwice

@caipengbo caipengbo Awaiting requested review from caipengbo

@git-hulk git-hulk Awaiting requested review from git-hulk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AntiTopQuark @git-hulk @mapleFU @PragmaTwice @caipengbo