libs: Call Flush() before rename #2428 by goolAdapter · Pull Request #2439 · tendermint/tendermint

goolAdapter · 2018-09-19T03:27:22Z

fix Group.RotateFile need call Flush() before rename. #2428

codecov-io · 2018-09-19T03:35:51Z

Codecov Report

Merging #2439 into develop will increase coverage by 0.05%.
The diff coverage is 54.34%.

@@             Coverage Diff             @@
##           develop    #2439      +/-   ##
===========================================
+ Coverage    61.28%   61.33%   +0.05%     
===========================================
  Files          198      198              
  Lines        16383    16397      +14     
===========================================
+ Hits         10040    10057      +17     
+ Misses        5508     5504       -4     
- Partials       835      836       +1

Impacted Files	Coverage Δ
consensus/state.go	`77.27% <100%> (+0.71%)`	⬆️
consensus/wal.go	`62.79% <100%> (+1.55%)`	⬆️
consensus/wal_generator.go	`79.43% <46.66%> (-1.15%)`	⬇️
libs/autofile/group.go	`65.54% <53.57%> (-1.22%)`	⬇️
blockchain/pool.go	`65.73% <0%> (-0.7%)`	⬇️
consensus/reactor.go	`73.82% <0%> (+0.8%)`	⬆️
libs/autofile/autofile.go	`76.05% <0%> (+1.4%)`	⬆️

melekes

Thank you for your contribution! Awesome work. I've left a few comments (mostly related to tests) which need to be addressed. After those are fixed, we can merge it

melekes · 2018-09-19T07:32:10Z

consensus/wal_test.go

+		t.Fatal(err)
+	}
+
+	time.Sleep(5 * time.Second) //wait groupCheckDuration, make sure RotateFile run


We're trying to avoid long sleep calls in our tests. Do you think we can do something with this call? Maybe make groupCheckDuration a configuration variable

# libs/autofile/group.go type Config struct { GroupCheckDuration time.Duration } func OpenGroup(headPath string, cfg Config) (g *Group, err error) { # consensus/wal.go type Config struct { GroupConfig } func NewWAL(walFile string, cfg Config) (*baseWAL, error) {

Not only the GroupCheckDuration, but HeadSizeLimit also need to config, this config filed seems only need changed by test. I added some member func to Group, left NewWAL concise.

melekes · 2018-09-19T07:32:57Z

consensus/wal_test.go

+	if err != nil {
+		panic(fmt.Errorf("failed to create temp WAL file: %v", err))
+	}
+


defer os.RemoveAll(walDir) ?

melekes · 2018-09-19T07:33:36Z

consensus/wal_test.go

+		t.Fatal(err)
+	}
+
+	wal.group.SetHeadSizeLimit(4096) //cause fragment


again, better to pass as a config option (although this requires some work)

melekes · 2018-09-19T07:38:00Z

consensus/wal_test.go

+
+	time.Sleep(5 * time.Second) //wait groupCheckDuration, make sure RotateFile run
+
+	wal.group.Flush()


wal.Group().Flush()

generally you should not access private fields / methods (even though you can) - sign that there's something wrong with your API

melekes · 2018-09-19T07:41:43Z

CHANGELOG_PENDING.md


 BUG FIXES:
 - [node] \#2294 Delay starting node until Genesis time
+- [autofile] \#2428 Group.RotateFile need call Flush() before rename.


Could you please add your github username in the end? before rename (@goolAdapter)

melekes · 2018-09-19T10:14:03Z

libs/autofile/group.go

 }

+// SetGroupCheckDuration allows you to overwrite default groupCheckDuration, this must call before Start().
+func (g *Group) SetGroupCheckDuration(duration time.Duration) {


please don't. let's use config for this

melekes · 2018-09-19T10:17:04Z

consensus/wal_test.go


-	wal.group.SetHeadSizeLimit(4096) //cause fragment
+	//this magic number 4K can truncate the content when RotateFile. defaultHeadSizeLimit(10M) is hard to simulate.
+	wal.Group().SetHeadSizeLimit(4096)


same here. we shouldn't expose SetXXX methods for something which is not supposed to change during the WAL group lifetime.

SetHeadSizeLimit is exposed before this PR，and was used in logjack cmd，should first keep consistent , refactor later? or I refactor it at the same time?

We would appreciate if you refactor this part! Technical debt tends to accumulate over time, so we need to refactor code sometimes to maintain Tendermint in good shape continually.

melekes · 2018-09-20T05:40:12Z

libs/autofile/cmd/logjack.go


 	// Open Group
-	group, err := auto.OpenGroup(headPath)
+	group, err := auto.OpenGroup(headPath, auto.SetHeadSizeLimit(chopSize), auto.SetTotalSizeLimit(limitSize))


if these are options now, they must not have a Set prefix

yes， Set* -> With* looks better?

GroupHeadSizeLimit()
GroupTotalSizeLimit()

consensus/wal.go

libs/autofile/group.go

melekes

Cool, thanks!

xla · 2018-09-25T10:44:00Z

@goolAdapter Could you resolve the conflicts with develop then this should be good to go.

fix Group.RotateFile need call Flush() before rename. tendermint#2428

079b997

goolAdapter requested review from ebuchman, melekes and xla as code owners September 19, 2018 03:27

melekes suggested changes Sep 19, 2018

View reviewed changes

melekes reviewed Sep 19, 2018

View reviewed changes

fix some review issue. tendermint#2428

4e3ed8b

melekes reviewed Sep 19, 2018

View reviewed changes

goolAdapter added 4 commits September 20, 2018 09:58

refactor Group's config: replace setting member with initial option

51ef73c

fix a handwriting mistake

57223f3

fix a time window error between rename and write.

4fb806f

fix a syntax mistake.

dd438a0

melekes reviewed Sep 20, 2018

View reviewed changes

goolAdapter added 3 commits September 20, 2018 14:21

change option name Get_ to With_

b7e17ec

fix review issue

6924066

fix review issue

03af490

melekes approved these changes Sep 20, 2018

View reviewed changes

goolAdapter mentioned this pull request Sep 20, 2018

ConsensusState.handleMsg print detail error msg #2262

Closed

xla self-assigned this Sep 25, 2018

xla changed the title ~~fix Group.RotateFile need call Flush() before rename. #2428~~ libs: Call Flush() before rename. #2428 Sep 25, 2018

xla changed the title ~~libs: Call Flush() before rename. #2428~~ libs: Call Flush() before rename #2428 Sep 25, 2018

Merge branch 'develop' into fix_group_flush

e0a5cab

xla merged commit 110b07f into tendermint:develop Sep 25, 2018


		time.Sleep(5 * time.Second) //wait groupCheckDuration, make sure RotateFile run

		wal.group.Flush()

Conversation

goolAdapter commented Sep 19, 2018

Uh oh!

codecov-io commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goolAdapter Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

xla commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Sep 19, 2018 •

edited

Loading

goolAdapter Sep 19, 2018 •

edited

Loading

melekes Sep 19, 2018 •

edited

Loading