Skip to content
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

storage: Use compact.NodeID #2395

Merged
merged 2 commits into from
Mar 11, 2021
Merged

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Mar 11, 2021

The old NodeID type is very complex. This change replaces it with
the simple compact.NodeID from the application layer all the way down
to SubtreeCache, but the latter still uses the old type. In follow-up
changes it will be removed completely.

Part of #2378.

Checklist

@pav-kv pav-kv requested review from Martin2112 and pphaneuf March 11, 2021 14:44
@pav-kv pav-kv marked this pull request as ready for review March 11, 2021 14:44
@pav-kv pav-kv requested a review from a team as a code owner March 11, 2021 14:44
@pav-kv pav-kv force-pushed the use_compact_node_id branch 2 times, most recently from 6288fa7 to e2e35b2 Compare March 11, 2021 14:56
@pav-kv pav-kv changed the title Use compact.NodeID storage: Use compact.NodeID Mar 11, 2021
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #2395 (29a7425) into master (a431457) will increase coverage by 0.01%.
The diff coverage is 51.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2395      +/-   ##
==========================================
+ Coverage   65.76%   65.77%   +0.01%     
==========================================
  Files         107      107              
  Lines        7758     7749       -9     
==========================================
- Hits         5102     5097       -5     
+ Misses       2122     2120       -2     
+ Partials      534      532       -2     
Impacted Files Coverage Δ
storage/cloudspanner/tree_storage.go 34.92% <0.00%> (ø)
storage/memory/log_storage.go 3.50% <0.00%> (ø)
storage/memory/tree_storage.go 2.00% <0.00%> (ø)
storage/tools/dump_tree/dumplib.go 41.59% <0.00%> (+0.34%) ⬆️
storage/tree/node.go 94.61% <ø> (ø)
storage/cache/subtree_cache.go 54.00% <36.36%> (-0.17%) ⬇️
log/sequencer.go 75.72% <71.42%> (+2.33%) ⬆️
server/proof_fetcher.go 92.30% <100.00%> (+6.10%) ⬆️
storage/mysql/log_storage.go 65.48% <100.00%> (ø)
storage/mysql/tree_storage.go 49.74% <100.00%> (-1.04%) ⬇️
... and 2 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 a431457...29a7425. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
storage/cache/subtree_cache_test.go Outdated Show resolved Hide resolved
storage/cache/subtree_cache_test.go Outdated Show resolved Hide resolved
return nodes, nil
}

// TODO(pavelkalinnikov): Allow nodes to be out of order.
func nodesAreEqual(lhs []stree.Node, rhs []stree.Node) error {
func nodesAreEqual(lhs, rhs []stree.Node) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this with some variant of cmp.Diff? That did't exist when this was written + same for func below.

Copy link
Contributor Author

@pav-kv pav-kv Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to significantly refactor/simplify SubtreeCache anyway, so I will likely revisit/delete this bit in follow-up PRs.

@pav-kv pav-kv force-pushed the use_compact_node_id branch from e2e35b2 to 0417598 Compare March 11, 2021 16:45
@pav-kv pav-kv force-pushed the use_compact_node_id branch from 0417598 to 29a7425 Compare March 11, 2021 16:48
@pav-kv pav-kv merged commit fa9aa84 into google:master Mar 11, 2021
@pav-kv pav-kv deleted the use_compact_node_id branch March 11, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants