chore: update CI toolchain and clean up code #244

tabokie · 2022-07-15T04:05:34Z

Ref #235

Update toolchain to 2022-07-13, add a makefile.

A lot of minor refactoring:

Refactor scan_messages interface to share more code paths, and add tests for them.
Hide LogFileFormat from outer crate, it should stay an implementation detail of file_pipe_log.
Change DataLayout to u64, because otherwise it needs additional care to make sure Alignment(0) and NoAlignment don't coexist.
Remove some tests that aren't very useful.

Signed-off-by: tabokie [email protected]

Signed-off-by: tabokie <[email protected]>

codecov · 2022-07-15T05:04:45Z

Codecov Report

Merging #244 (f28f309) into master (c3a6156) will increase coverage by 0.44%.
The diff coverage is 99.22%.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   97.24%   97.68%   +0.44%     
==========================================
  Files          30       30              
  Lines       10951    10715     -236     
==========================================
- Hits        10649    10467     -182     
+ Misses        302      248      -54

Impacted Files	Coverage Δ
src/env/mod.rs	`100.00% <ø> (ø)`
src/lib.rs	`100.00% <ø> (ø)`
src/swappy_allocator.rs	`99.23% <0.00%> (ø)`
src/engine.rs	`97.90% <98.23%> (+0.99%)`	⬆️
src/file_pipe_log/pipe.rs	`99.24% <98.93%> (+0.08%)`	⬆️
src/config.rs	`97.50% <100.00%> (ø)`
src/file_pipe_log/format.rs	`99.51% <100.00%> (+0.58%)`	⬆️
src/file_pipe_log/log_file.rs	`100.00% <100.00%> (+0.43%)`	⬆️
src/file_pipe_log/mod.rs	`98.46% <100.00%> (+0.88%)`	⬆️
src/file_pipe_log/pipe_builder.rs	`96.20% <100.00%> (-0.04%)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a6156...f28f309. Read the comment docs.

Signed-off-by: tabokie <[email protected]>

…anup-220715 Signed-off-by: tabokie <[email protected]>

Signed-off-by: tabokie <[email protected]>

tabokie · 2022-08-03T08:00:19Z

/cc @LykxSassinator

Signed-off-by: tabokie <[email protected]>

Connor1996

LGTM

LykxSassinator

Rest LGTM

LykxSassinator · 2022-08-03T09:47:23Z

Makefile

+## Run tests.
+ifeq ($(WITH_STABLE_TOOLCHAIN), true)
+test:
+	cargo test --all --features all_stable_except_failpoints ${EXTRA_CARGO_ARGS} -- --nocapture


Recommend to add extra --color always, making the results both on successful and failed tests more readable.

Do you encounter any problem using it? I think the default auto should work good enough.

Actually，it's work fine in my environment, also on MacOS.

LykxSassinator · 2022-08-03T09:49:00Z

Makefile

+	cargo test --test failpoints --features all_stable ${EXTRA_CARGO_ARGS} -- --test-threads 1 --nocapture
+else
+test:
+	cargo test --all --features all_except_failpoints ${EXTRA_CARGO_ARGS} -- --nocapture


src/file_pipe_log/format.rs

LykxSassinator · 2022-08-03T10:42:31Z

src/file_pipe_log/reader.rs

+    ) -> Result<()> {
+        self.valid_offset = LogFileFormat::encode_len(format.version);
+        self.file_id = Some(file_id);
+        self.format = Some(format);


How about still using LogFileContext to represent the basics of file metadata ?

Similar to my other comment, because the context no longer holds alignment, using it doesn't make sense here.

LykxSassinator · 2022-08-03T10:57:17Z

src/pipe_log.rs

    }
 }

 #[derive(Debug, Clone)]
 pub struct LogFileContext {
    pub id: FileId,
-    pub format: LogFileFormat,
+    pub version: Version,


Why not introduce LogFileFormat as an integrated representation of file metadata? Version is just a slice of metadata in the log, but LogFileFormat owns the whole.

Hide LogFileFormat from outer crate, it should stay an implementation detail of file_pipe_log.

There's no need to expose it because only "file_pipe_log" needs to know about alignment.

LykxSassinator · 2022-08-03T11:02:19Z

src/file_pipe_log/log_file.rs

+            handle,
+            reader,
+            // Set to an invalid offset to force a reseek at first read.
+            offset: u64::MAX,


I've got the purpose that u refactor the LogFileReader and its related method build_file_reader. But referring to the design on LogFileWriter, I still recommend to make the parse_format integrated into the processing of LogFileReader::open(), rather than open() -> manually parse_format by callers.

My change removes the format stored inside reader, because reader doesn't need a format to function. The writer, on the other hand, needs to do format-specific initialization, so we can't refactor it like reader.

src/file_pipe_log/format.rs

LykxSassinator · 2022-08-03T15:46:21Z

Makefile

+	cargo clippy --all --features all_stable --all-targets -- -D clippy::all
+else
+clippy:
+	cargo clippy --all --all-features --all-targets -- -D clippy::all


Here, don't we need +nightly trait any more when ${WITH_STABLE_TOOLCHAIN} == false ?

I've tested the makefile on Centos:
Linux version 3.10.0-862.14.4.el7.x86_64 ([email protected]) (gcc version 4.8.5 20150623 (Red Hat 4.8.5-28) (GCC) ) #1 SMP Wed Sep 26 15:12:11 UTC 2018

And it returns the following failure:

error[E0554]: `#![feature]` may not be used on the stable release channel --> src/lib.rs:16:34 | 16 | #![cfg_attr(feature = "nightly", feature(test))] | ^^^^^^^^^^^^^ error[E0554]: `#![feature]` may not be used on the stable release channel --> src/lib.rs:17:31

And if I add the +nightly trait, both make test and make clippy worked as expectation without errs, by setting ${WITH_STABLE_TOOLCHAIN} == false .
Also, the feature specification on all_except_failpoints in cargo.toml shows that we need +nightly.

The idea is to assume user has set the correct default toolchain. But force setting sounds okay too, I'll add it later.

update CI toolchain and improve scan interfaces

9747bcf

Signed-off-by: tabokie <[email protected]>

tabokie changed the title ~~chore: update CI toolchain and clean up code~~ chore: update CI toolchain and reseek after write failure Jul 15, 2022

restore failpoint

01e0bd9

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the cleanup-220715 branch from 99af65f to 01e0bd9 Compare July 15, 2022 04:40

tabokie mentioned this pull request Jul 15, 2022

cargo +nightly clippy --all --all-features --all-targets -- -D clippy::all error #235

Open

tabokie changed the title ~~chore: update CI toolchain and reseek after write failure~~ chore: update CI toolchain and clean up code Jul 15, 2022

tabokie added 5 commits July 15, 2022 18:20

Merge branch 'master' into cleanup-220715

59064ba

Merge branch 'master' into cleanup-220715

03a8469

clean up

02ed068

Signed-off-by: tabokie <[email protected]>

refactor a bit and add makefile

9ec5a77

Signed-off-by: tabokie <[email protected]>

Merge branch 'master' of https://github.com/tikv/raft-engine into cle…

7ab1af2

…anup-220715 Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the cleanup-220715 branch from 16c518b to 47ce01b Compare August 2, 2022 05:39

add makefile

3faee0e

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the cleanup-220715 branch from ce4a20a to 3faee0e Compare August 2, 2022 10:30

tabokie mentioned this pull request Aug 3, 2022

Unify the strategy of tolerating tail corruption when Version == V2. #255

Closed

refactors

557d086

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the cleanup-220715 branch 2 times, most recently from 240d7bc to b873cce Compare August 3, 2022 08:16

test fmt works

f28f309

Signed-off-by: tabokie <[email protected]>

tabokie force-pushed the cleanup-220715 branch from b873cce to f28f309 Compare August 3, 2022 08:18

tabokie requested review from 5kbpers and Connor1996 August 3, 2022 08:58

Connor1996 approved these changes Aug 3, 2022

View reviewed changes

tabokie merged commit 6a6fe3b into tikv:master Aug 3, 2022

LykxSassinator reviewed Aug 3, 2022

View reviewed changes

src/file_pipe_log/format.rs Show resolved Hide resolved

LykxSassinator reviewed Aug 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update CI toolchain and clean up code #244

chore: update CI toolchain and clean up code #244

tabokie commented Jul 15, 2022 •

edited

Loading

codecov bot commented Jul 15, 2022 •

edited

Loading

tabokie commented Aug 3, 2022

Connor1996 left a comment

LykxSassinator left a comment

LykxSassinator Aug 3, 2022

tabokie Aug 3, 2022

LykxSassinator Aug 3, 2022 •

edited

Loading

LykxSassinator Aug 3, 2022

LykxSassinator Aug 3, 2022

tabokie Aug 3, 2022 •

edited

Loading

LykxSassinator Aug 3, 2022

tabokie Aug 3, 2022

LykxSassinator Aug 3, 2022

tabokie Aug 3, 2022

LykxSassinator Aug 3, 2022 •

edited

Loading

tabokie Aug 4, 2022

chore: update CI toolchain and clean up code #244

chore: update CI toolchain and clean up code #244

Conversation

tabokie commented Jul 15, 2022 • edited Loading

codecov bot commented Jul 15, 2022 • edited Loading

Codecov Report

tabokie commented Aug 3, 2022

Connor1996 left a comment

Choose a reason for hiding this comment

LykxSassinator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LykxSassinator Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LykxSassinator Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie commented Jul 15, 2022 •

edited

Loading

codecov bot commented Jul 15, 2022 •

edited

Loading

LykxSassinator Aug 3, 2022 •

edited

Loading

tabokie Aug 3, 2022 •

edited

Loading

LykxSassinator Aug 3, 2022 •

edited

Loading