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

Making index record size configurable #459

Merged

Conversation

AnkurAnand11
Copy link
Contributor

@AnkurAnand11 AnkurAnand11 commented Nov 16, 2023

Change log description
Make index record size configurable. Current index record is of fixed length of 4k bytes. It would waste quite some space if real index is small. It is better to be adapted to the real size when reading/writing.

Purpose of the change
Closes #461

What the code does
Created a new method get_record_size in the fields trait of mod.rs with default implementation of 4k bytes. If a different record size value is required, the traits method can be overridden while writing the index event.

How to verify it
All test cases should pass.

Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
src/index/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

Merging #459 (511bcda) into master (c0f1813) will decrease coverage by 0.20%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   72.78%   72.59%   -0.20%     
==========================================
  Files          49       49              
  Lines       12390    12386       -4     
  Branches    12390    12386       -4     
==========================================
- Hits         9018     8991      -27     
- Misses       3372     3395      +23     
Files Coverage Δ
src/index/mod.rs 96.82% <100.00%> (ø)

... and 12 files with indirect coverage changes

Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
@AnkurAnand11 AnkurAnand11 marked this pull request as ready for review November 30, 2023 03:44
Copy link
Member

@tkaitchuck tkaitchuck left a comment

Choose a reason for hiding this comment

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

Where does 111 come from? is there a Java PR that defines it?

@AnkurAnand11
Copy link
Contributor Author

Where does 111 come from? is there a Java PR that defines it?

We have hardcoded the attribute Id here to 111 since randomly generating it was causing issue while reading.

src/index/writer.rs Outdated Show resolved Hide resolved
src/index/writer.rs Outdated Show resolved Hide resolved
@tkaitchuck
Copy link
Member

Where does 111 come from? is there a Java PR that defines it?

We have hardcoded the attribute Id here to 111 since randomly generating it was causing issue while reading.

These should be defined in the Attributes class in java.

Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
src/index/writer.rs Outdated Show resolved Hide resolved
src/index/writer.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
}
}
_ => {
info!("get segment attribute for record_size failed due to {:?}", reply);
Copy link
Member

Choose a reason for hiding this comment

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

Humm, this actually brings up an interesting edge case:
Suppose the user creates the reader before the writer. Then we would get a segment does not exist error. In this case we have a couple of options: We could fail and simply not allow creating the reader first.
To make sure the fallback value is correct, we should add a check on reader:350 to make sure the value read was actually the correct size.

Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
Signed-off-by: Ankur_Anand <Ankur.Anand@dell.com>
@tkaitchuck tkaitchuck merged commit bbe047a into pravega:master Dec 14, 2023
14 checks passed
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.

Make index record size configurable
3 participants