Remove legacy NodeID #2404
Remove legacy NodeID #2404
Conversation
pav-kv
force-pushed
the
rm_node_id
branch
2 times, most recently
from
acf7b2f to
fcdfb25
Compare
Codecov Report
@@ Coverage Diff @@
## master #2404 +/- ##
==========================================
- Coverage 65.82% 65.50% -0.32%
==========================================
Files 106 104 -2
Lines 7670 7515 -155
==========================================
- Hits 5049 4923 -126
+ Misses 2095 2071 -24
+ Partials 526 521 -5
Continue to review full report at Codecov.
|
| ) | ||
|
|
||
| // NodeStorage provides an interface for storing and retrieving subtrees. | ||
| type NodeStorage interface { | ||
| GetSubtree(n tree.NodeID) (*storagepb.SubtreeProto, error) | ||
| GetSubtree(prefix []byte) (*storagepb.SubtreeProto, error) |
There was a problem hiding this comment.
Unfortunately golang has no immutable byte string type. There could be value in using string instead. OTOH the original NodeID type was not immutable and I suspect it's not worth doing.
There was a problem hiding this comment.
Within the scope of this PR, the safety remains the same as it was. I might improve it in follow-ups.
| // This should never happen - we should've already read all the data we | ||
| // need above, in Preload() | ||
| glog.Warningf("Unexpectedly reading from within getNodeHash(): %s", n.String()) | ||
| ret, err := getSubtrees([]tree.NodeID{n}) | ||
| glog.Warningf("Unexpectedly reading from within getNodeHash(): %x", id) |
There was a problem hiding this comment.
I think we previously found the above comment to be incorrect. Might demote the log level. This condition is also not tested. Again, I suspect that was true before.
There was a problem hiding this comment.
I believe it's correct (or don't see why it's not). Also, I found 0 occurrences of this warning in our logs. I think we should rather return an error here, or don't have this callback in the first place. To be addressed in follow-ups.
There was a problem hiding this comment.
Maybe it was maps again? I remember we tried to remove it before and it did happen.
This reduces dependency on NodeID, but breaks SubtreeCache, because it still uses NodeID. This is fixed in the next commit.
This migrates SubtreeCache from NodeID. Storage implementations still pass callbacks using NodeID, this is fixed in the next commits.
|
Left pre-existing coverage/safety things for future addressing. For now I can see that |
This change eliminates the last cases of using
NodeID, most notably inSubtreeCache,and removes this type. Now
compact.NodeIDis used all the way down to storage, butstorage implementations still use
[]byteslices to address tiles, for backward compatibilitywith the existing databases.
Review commit by commit is recommended.
Part of #2378
Checklist