-
Notifications
You must be signed in to change notification settings - Fork 328
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
[db]log mptrie node #3634
[db]log mptrie node #3634
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3634 +/- ##
==========================================
+ Coverage 74.47% 75.07% +0.60%
==========================================
Files 269 299 +30
Lines 23925 25202 +1277
==========================================
+ Hits 17818 18921 +1103
- Misses 5174 5302 +128
- Partials 933 979 +46
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
how does this pr solve #3204 ? |
* check nil for response.EpochData (#3642) * remove unused db/sql (#3645) * [ioctl] Build Read unittest (#3635) * build Read unittest * fix code smells * remove redundant argument * fix error * remove redundant code * remove redundant space * delete useless code Co-authored-by: huof6890 <[email protected]> * [pkg] rotate log file (#3638) * add lumberjack * add lograte conf in Dockerfile * del space line * [ioctl] Build did geturi command line into new ioctl (#3650) * [ioctl] build did geturi command line into new ioctl * add test data * move abi reader to init * build unittest to cover the modification * move global var _didABI to did.go * move abi reader to did * alter did string with did prefix * rename did string Co-authored-by: huof6890 <[email protected]> * [ioctl] build did gethash command line into new ioctl (#3639) * [ioctl] build did gethash command line into new ioctl * remove redundant code * build unittest to cover the modification * remove redundant args * add command description * modify test value * remove func encodeGet * remove func encodeGet * build unittest to cover the modification * reset default gas limit * use fault address from identityset * remove redundant space * update command name * move abi reader to init * move abi reader to did * alter did string with did prefix * rename did string Co-authored-by: huofei <[email protected]> * [factory] add error details (#3657) Co-authored-by: huofei <[email protected]> Co-authored-by: Jeremy Chou <[email protected]> Co-authored-by: CoderZhi <[email protected]>
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
db/trie/mptrie/lognode.go
Outdated
} | ||
|
||
// ParseNodeEvent parse the node event | ||
func ParseNodeEvent(b []byte) (NodeEvent, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to unit test and rename to private function
db/trie/mptrie/lognode.go
Outdated
return logFile.Close() | ||
} | ||
|
||
func logNode(n node) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameters:
- node type,
leaf
,extension
, andbranch
- action type, e.g.,
read
andwrite
, orsearch
,delete
, andnew
- hashvalue, creating via cacheNode.Hash()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create hashvalue
inside of logNode
function. Other than that, it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
defer func() { | ||
if err = mptrie.CloseLogDB(); err != nil { | ||
log.L().Error("Failed to close mptrie log DB.", zap.Error(err)) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function will be called right away. It seems that the PR has not been fully tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has been tested OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know. I have wrong understanding of defer
@@ -55,15 +54,14 @@ func newExtensionNodeFromProtoPb(pb *triepb.ExtendPb, hashVal []byte) *extension | |||
child: newHashNode(pb.Value), | |||
} | |||
e.cacheNode.serializable = e | |||
if err := logNode(_nodeTypeExtension, _actionTypeNew, hashVal, e); err != nil { | |||
if err := logNode(_nodeTypeExtension, _actionTypeNew, e, nil); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not working here
@@ -93,7 +91,7 @@ func newBranchNodeFromProtoPb(pb *triepb.BranchPb, hashVal []byte) *branchNode { | |||
} | |||
bnode.indices = NewSortedList(bnode.children) | |||
bnode.cacheNode.serializable = bnode | |||
if err := logNode(_nodeTypeBranch, _actionTypeNew, hashVal, bnode); err != nil { | |||
if err := logNode(_nodeTypeBranch, _actionTypeNew, bnode, nil); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not working here
@@ -55,7 +54,7 @@ func newLeafNodeFromProtoPb(pb *triepb.LeafPb, hashVal []byte) *leafNode { | |||
value: pb.Value, | |||
} | |||
l.cacheNode.serializable = l | |||
if err := logNode(_nodeTypeLeaf, _actionTypeNew, hashVal, l); err != nil { | |||
if err := logNode(_nodeTypeLeaf, _actionTypeNew, l, nil); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not working here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too at first.
see the following code, first, it will check hashVal
, only use cli if hashVal
is not set.
iotex-core/db/trie/mptrie/cachenode.go
Lines 25 to 32 in 715ccb7
func (cn *cacheNode) hash(cli client, flush bool) ([]byte, error) { | |
if len(cn.hashVal) != 0 { | |
return cn.hashVal, nil | |
} | |
if cli == nil { | |
return []byte{}, errors.New("client cannot be nil") | |
} | |
pb, err := cn.proto(cli, flush) |
db/trie/mptrie/lognode_test.go
Outdated
} | ||
if event.KeyLen != test.event.KeyLen { | ||
t.Errorf("unexpected key length %v", event.KeyLen) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use same test code style? like
require.NoError(err)
require.True(event.NodeType == test.event.NodeType)
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
@@ -85,6 +91,9 @@ func newBranchNodeFromProtoPb(pb *triepb.BranchPb, hashVal []byte) *branchNode { | |||
} | |||
bnode.indices = NewSortedList(bnode.children) | |||
bnode.cacheNode.serializable = bnode | |||
if err := logNode(_nodeTypeBranch, _actionTypeNew, bnode, nil); err != nil { | |||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is panic
used when logging fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newBranchNodeFromProtoPb
does not handle errors, directly returns *branchNode
, logNode should not fail here either
@@ -69,6 +72,9 @@ func newRootBranchNode(cli client, children map[byte]node, indices *SortedList, | |||
} | |||
} | |||
} | |||
if err := logNode(_nodeTypeBranch, _actionTypeNew, bnode, cli); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is logging
used at the end of function instead of at the head?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait for the bnode
Initialized
@@ -0,0 +1,127 @@ | |||
package mptrie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the name lognode.go
is misleading. log.go
is a better choice
type actionType byte | ||
|
||
const ( | ||
_nodeTypeLeaf nodeType = 'l' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a byte array [10]byte
be used here? The meaning of the data as a byte
is hard to read
return err | ||
} | ||
// write event body | ||
_, err = logWriter.Write(event.Bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to read the binary data in the db? Do we have the tool or component to print the data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A byte is used to save the size of the binrary. Binary itself is not readable and needs to be processed by tool. In the lognode_test.go, there is a parseNodeEvent that can parse the binrary.
How the tools are provided needs to be discussed (print, database)
Description
record mptrie node infomation to postgres DB.
b
ranchNodee
xtensionNodel
eafNodeFixes #3204
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: