add checksum check in ManagedDirectory #605
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
- Coverage 91.03% 91.01% -0.02%
==========================================
Files 172 173 +1
Lines 17838 17990 +152
==========================================
+ Hits 16239 16374 +135
- Misses 1599 1616 +17
Continue to review full report at Codecov.
|
fulmicoton
reviewed
Jul 27, 2019
fulmicoton
reviewed
Jul 27, 2019
fulmicoton
reviewed
Jul 27, 2019
fulmicoton
reviewed
Jul 27, 2019
trinity-1686a
commented
Jul 29, 2019
fulmicoton
reviewed
Aug 11, 2019
fulmicoton
reviewed
Aug 11, 2019
fulmicoton
reviewed
Aug 11, 2019
fulmicoton
reviewed
Aug 11, 2019
this is more of a poc, it require some refactoring into multiple files `terminate(self)` is implemented, but not used anywhere yet
trinity-1686a
force-pushed
the
checksum
branch
from
07c819a to
a4c47aa
Compare
use BitOrder for integer to raw byte conversion consider atomic write imply atomic read, which might not actually be true use some indirection to have a boxable terminating writer
…ould add dependancy to drop_bomb to help find where terminate() should be called implement TerminatingWrite for wrapper writers make tests pass /!\ some tests seems to pass where they shouldn't
trinity-1686a
force-pushed
the
checksum
branch
from
e6c0c80 to
e3b0cbe
Compare
trinity-1686a
force-pushed
the
checksum
branch
from
e3b0cbe to
32baf73
Compare
fulmicoton
reviewed
Sep 13, 2019
fulmicoton
reviewed
Sep 13, 2019
src/directory/managed_directory.rs
Outdated
| let index_version = footer[0]; | ||
| match index_version { | ||
| 0 => Footer::V0(V0::from_bytes(footer)), | ||
| _ => panic!("unsuported index_version"), |
There was a problem hiding this comment.
Maybe return an error?
There was a problem hiding this comment.
Can you add a TODO to add a "string version", so that the error message in a backward incompatibility scenario becomes more helpful?
fulmicoton
reviewed
Sep 13, 2019
fulmicoton
reviewed
Sep 13, 2019
src/directory/managed_directory.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone)] | ||
| pub enum Footer { |
There was a problem hiding this comment.
I think there are stuff we can force for all versions...
Should we use a
pub struct Footer {
version_configuration: VersionConfiguration,
meta: String //< human readable with information,
}
and put the Version specific information in a separate enum.
If you want we can do that in a separate PR. In that case please add a TODO.
fulmicoton
reviewed
Sep 13, 2019
fulmicoton
reviewed
Sep 17, 2019
fulmicoton
reviewed
Sep 17, 2019
fulmicoton
reviewed
Sep 17, 2019
fulmicoton
reviewed
Sep 17, 2019
fulmicoton
reviewed
Sep 17, 2019
fulmicoton
reviewed
Sep 17, 2019
|
@fdb-hiroshima I am very happy with all of the choice you made. I added a couple of nitpicks. No need to address all of them but I think there could be more unit tests especially for serialization/deserialization. Could you also add your contribution in the CHANGELOG? |
fulmicoton
reviewed
Sep 18, 2019
| let tantivy_patch = LittleEndian::read_u32(&footer[size - 8..]); | ||
| Ok(Footer { | ||
| tantivy_version: (tantivy_major, tantivy_minor, tantivy_patch), | ||
| meta: String::from_utf8_lossy(&footer[size - meta_len - 20..size - 20]).into_owned(), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.managed.jsonis not checksummed as it does not write itself through ManagedDirectory, but meta.json does (which but an little bit of binary at the end of an otherwise ascii file).fix #400