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

Allow for a same-thread doc compressor. #1510

Merged
merged 1 commit into from Sep 13, 2022
Merged

Conversation

fulmicoton
Copy link
Collaborator

In addition, it isolates the doc compressor logic, better reports io::Result.

In the case of the same-thread doc compressor,
the blocks are also not copied.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2022

Codecov Report

Merging #1510 (22b5d11) into main (4d634d6) will increase coverage by 0.02%.
The diff coverage is 98.45%.

@@            Coverage Diff             @@
##             main    #1510      +/-   ##
==========================================
+ Coverage   93.96%   93.98%   +0.02%     
==========================================
  Files         242      243       +1     
  Lines       44936    45089     +153     
==========================================
+ Hits        42226    42379     +153     
  Misses       2710     2710              
Impacted Files Coverage Δ
src/store/store_compressor.rs 97.53% <97.53%> (ø)
src/core/index_meta.rs 95.17% <100.00%> (+0.81%) ⬆️
src/indexer/segment_serializer.rs 98.27% <100.00%> (+0.19%) ⬆️
src/indexer/segment_writer.rs 96.42% <100.00%> (+0.01%) ⬆️
src/store/mod.rs 99.23% <100.00%> (+0.05%) ⬆️
src/store/reader.rs 82.26% <100.00%> (ø)
src/store/writer.rs 100.00% <100.00%> (ø)
src/fastfield/multivalued/mod.rs 97.92% <0.00%> (+0.51%) ⬆️
ownedbytes/src/lib.rs 98.63% <0.00%> (+1.36%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fulmicoton fulmicoton force-pushed the same-thread-doc-compressor branch 3 times, most recently from 456e0db to dbd1ddd Compare September 8, 2022 01:28
@fulmicoton fulmicoton marked this pull request as ready for review September 8, 2022 01:32
@fulmicoton fulmicoton force-pushed the same-thread-doc-compressor branch 3 times, most recently from f02ff44 to 22b5d11 Compare September 8, 2022 01:40
@@ -235,6 +235,14 @@ impl InnerSegmentMeta {
}
}

fn return_true() -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn return_true() -> bool {
fn docstore_compress_dedicated_thread() -> bool {

/// If set to true, docstore compression will happen on a dedicated thread.
/// (defaults: true)
#[doc(hidden)]
#[serde(default = "return_true")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[serde(default = "return_true")]
#[serde(default = "docstore_compress_dedicated_thread")]

Comment on lines 18 to 20
enum Impls {
SameThread(BlockCompressorImpl),
DedicatedThread(DedicatedThreadBlockCompressorImpl),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the naming could be improved here a little

Suggested change
enum Impls {
SameThread(BlockCompressorImpl),
DedicatedThread(DedicatedThreadBlockCompressorImpl),
enum BlockCompressorVariants {
SameThread(SameThreadBlockCompressor),
DedicatedThread(DedicatedThreadBlockCompressor),

Copy link
Contributor

@PSeitz PSeitz left a comment

Choose a reason for hiding this comment

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

LGTM except some naming comments

In addition, it isolates the doc compressor logic,
better reports io::Result.

In the case of the same-thread doc compressor,
the blocks are also not copied.
@fulmicoton fulmicoton merged commit 817225e into main Sep 13, 2022
@fulmicoton fulmicoton deleted the same-thread-doc-compressor branch September 13, 2022 06:32
This was referenced Jan 13, 2023
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.

None yet

3 participants