[ledger] Ledger cleanups + make a test async #1073

huitseeker · 2021-08-02T21:43:32Z

found while trying to read about / dig into the WAL:

removes unneeded byte conversions,
removes unused, untested functions either in test utils or for which there's a good alternative,
makes Test_Compactor/Compactor_creates_checkpoints_eventually asynchronous but deterministic,
(introduces a minimalistic Observable interface that I know I'll reuse elsewhere, though not in this PR, see Networking test races #1020 )
Simplifies Compactor's start / stop slightly, thanks to @smnzhu 's LifeCycleManager

synzhu · 2021-08-03T06:04:58Z

ledger/complete/wal/compactor.go

 	stopc        chan struct{}
-	wg           sync.WaitGroup
+	lm           *lifecycle.LifecycleManager


ledger/complete/wal/compactor.go

ledger/complete/wal/compactor_test.go

ledger/complete/wal/compactor.go

synzhu

LGTM

synzhu · 2021-08-03T17:08:45Z

ledger/complete/wal/compactor.go

+		for observer := range c.observers {
+			observer.OnComplete()
+		}
+		c.stopc <- struct{}{}


You may be interested in #1077, which will allow you to replace c.stopc as well :)

ramtinms · 2021-08-03T17:11:57Z

ledger/trie.go

@@ -289,7 +281,7 @@ func (p *TrieProof) String() string {
 	interimIndex := 0
 	for j := 0; j < int(p.Steps); j++ {
 		// if bit is set
-		if p.Flags[j/8]&(1<<int(7-j%8)) != 0 {


@tarakby is this change okey?

j is an integer, % has an integer codomain, and j % 8 <= 7 by specification

That's correct, but we can replace this by Bit(p.Falgs, j) using this tool function.

I remembered Tarak recommended something about this part of the code and I was not remembering the details, that's why I asked.

Thanks for flagging @ramtinms. You're right I made changes in the past to the Bit function.

ledger/complete/wal/compactor_test.go

m4ksio · 2021-08-03T20:51:27Z

module/observable/observable.go

@@ -0,0 +1,8 @@
+package observable
+
+type Observable interface {


I wonder if we maybe should use some established library for this - https://github.com/ReactiveX/RxGo
Now the usecase is simple but it might grow

I'm familiar with RxGo and very much in favor, but I don't think we have scope for using it fully as a consequence of this PR alone.

The Observable API I put in modules right now is forward-compatible.

ledger/complete/wal/compactor.go

tarakby

Thanks for the ledger tools and tests clean up!
Minor suggestions.

Out of curiosity, did you use a linter tool to find out the non-needed type casts or was it by manual review ?

tarakby · 2021-08-04T20:06:58Z

ledger/common/encoding/encoding.go

@@ -272,7 +272,7 @@ func DecodeValue(encodedValue []byte) (ledger.Value, error) {
 }

 func decodeValue(inp []byte) (ledger.Value, error) {
-	return ledger.Value(inp), nil
+	return inp, nil


We could delete the function in this case. It also never errors.

tarakby · 2021-08-04T20:20:41Z

ledger/trie.go

@@ -289,7 +281,7 @@ func (p *TrieProof) String() string {
 	interimIndex := 0
 	for j := 0; j < int(p.Steps); j++ {
 		// if bit is set
-		if p.Flags[j/8]&(1<<int(7-j%8)) != 0 {


That's correct, but we can replace this by Bit(p.Falgs, j) using this tool function.

huitseeker · 2021-08-04T22:39:33Z

@tarakby I blasted the ones my IDE highlighted, but if we want to put that in CI, golang-ci supports unconvert: https://golangci-lint.run/usage/linters/

m4ksio

Looks good now

superseded by defaultHashes at init

See //reactivex.io/documentation/observable.html

- introduces a most minima form of the Observable interface to modules/observable See: http://reactivex.io/documentation/observable.html - make the compactor test asynchronous

This reverts commit 845a00b. Reasons for reversal: - provided impetus for clearing out races (onflow#1073, onflow#1095) - network tests now pass with race detector on! - but those timings are preposterous

huitseeker requested review from m4ksio, ramtinms and tarakby August 2, 2021 21:43

huitseeker requested a review from AlexHentschel as a code owner August 2, 2021 21:43

huitseeker force-pushed the ledger-warts branch 2 times, most recently from 738f918 to bb1a8ea Compare August 2, 2021 21:53

synzhu reviewed Aug 3, 2021

View reviewed changes

ledger/complete/wal/compactor.go Outdated Show resolved Hide resolved

huitseeker force-pushed the ledger-warts branch from bb1a8ea to 52668c1 Compare August 3, 2021 13:47

synzhu approved these changes Aug 3, 2021

View reviewed changes

synzhu reviewed Aug 3, 2021

View reviewed changes

ramtinms reviewed Aug 3, 2021

View reviewed changes

m4ksio reviewed Aug 3, 2021

View reviewed changes

huitseeker force-pushed the ledger-warts branch from 52668c1 to c59c9cb Compare August 3, 2021 21:37

m4ksio reviewed Aug 4, 2021

View reviewed changes

ledger/complete/wal/compactor.go Outdated Show resolved Hide resolved

huitseeker force-pushed the ledger-warts branch from c59c9cb to ec51229 Compare August 4, 2021 19:20

tarakby reviewed Aug 4, 2021

View reviewed changes

m4ksio approved these changes Aug 5, 2021

View reviewed changes

huitseeker added 7 commits August 6, 2021 09:42

Remove needless byte conversions

0c1e83f

Remove unused GetDefaultHashes

cf5bf8a

superseded by defaultHashes at init

Remove 2x unused functions in testutils

ec1d12d

Refactor the Compactor's start / stop cycle to LifecycleManager

7030f31

Introduces a minimalistic Observer / Observable interface

299f3bb

See //reactivex.io/documentation/observable.html

Remove time-dependency (and flakiness) on the compactor test

e5e6435

- introduces a most minima form of the Observable interface to modules/observable See: http://reactivex.io/documentation/observable.html - make the compactor test asynchronous

Clean up after the simplification of decodeValue

cc95354

huitseeker force-pushed the ledger-warts branch from ec51229 to cc95354 Compare August 6, 2021 13:42

huitseeker merged commit 3029e6f into onflow:master Aug 6, 2021

huitseeker mentioned this pull request Aug 6, 2021

[ledger] Remove stopc in the compactor (using Lifecycle) #1093

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ledger] Ledger cleanups + make a test async #1073

[ledger] Ledger cleanups + make a test async #1073

huitseeker commented Aug 2, 2021

synzhu Aug 3, 2021

synzhu left a comment

synzhu Aug 3, 2021

ramtinms Aug 3, 2021

huitseeker Aug 3, 2021 •

edited

tarakby Aug 4, 2021

ramtinms Aug 4, 2021

tarakby Aug 4, 2021

m4ksio Aug 3, 2021

huitseeker Aug 3, 2021

tarakby left a comment •

edited

tarakby Aug 4, 2021

tarakby Aug 4, 2021

huitseeker commented Aug 4, 2021

m4ksio left a comment

		@@ -0,0 +1,8 @@
		package observable

		type Observable interface {

[ledger] Ledger cleanups + make a test async #1073

[ledger] Ledger cleanups + make a test async #1073

Conversation

huitseeker commented Aug 2, 2021

Choose a reason for hiding this comment

synzhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Aug 3, 2021 • edited

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

Choose a reason for hiding this comment

tarakby left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker commented Aug 4, 2021

m4ksio left a comment

Choose a reason for hiding this comment

huitseeker Aug 3, 2021 •

edited

tarakby left a comment •

edited