Sort nodes by Filename in GetSubTree #507
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#507
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/sort-tree-nodes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
S3 requires only ascending ordering, so only asc was supported: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
To support desc need to change traversing way.
Closes #335
812ab4c543
toa3d1e3cda4
WIP: Sort nodes by Filename in GetSubTreeto Sort nodes by Filename in GetSubTreea3d1e3cda4
to7247dd271e
@ -474,6 +475,10 @@ func getSubTree(ctx context.Context, srv TreeService_GetSubTreeServer, cid cidSD
if err != nil {
return err
}
children, err = sortByFilename(ctx, cid, b.GetTreeId(), children, b.GetOrderBy().GetDirection(), forest)
Why not do it in
GetChildren
?Because we have memory and boltdb forest implementation, but ordering will be the same.
So we can sort in both bolt and memory implementation, right?
Now
get children
returns nodeID, parentID and meta, so i removedTreeGetMeta
call from service.7247dd271e
toe258202bd4
@ -485,0 +508,4 @@
if filenameBytes, found := filenames[rhs]; found {
rightFilename = string(filenameBytes)
}
return leftFilename < rightFilename
Can we compare using
bytes.Compare
?looks so, thx. fixed.
e258202bd4
to8ae09e69c6
8ae09e69c6
to60af2616d3
60af2616d3
to335641a364
@ -174,0 +191,4 @@
break
}
if !errors.Is(err, pilorama.ErrTreeNotFound) {
e.reportShardError(sh, "can't perform `TreeGetMeta`", err,
Probably the error message should be
can't perform 'TreeGetAttributeValues'
method deleted
335641a364
to102a9df67e
@ -69,2 +69,4 @@
// do performs a single move operation on a tree.
func (s *memoryTree) do(op *Move) move {
m := op.Meta
if m.Items == nil {
To have the same behaviour as bolt forest.
102a9df67e
to2cc6976829
2cc6976829
to8e92e46463
@ -970,2 +971,4 @@
children = append(children, binary.LittleEndian.Uint64(k[9:]))
}
for _, childID := range children {
children
is not needed outside the transaction now. You can probably merge this loop with the previous one and get rid ofchildren
.fixed
@ -156,2 +156,2 @@
testGetChildren(t, 0, []uint64{10, 2, 7})
testGetChildren(t, 10, []uint64{3, 6})
testGetChildren(t, 0, []NodeInfo{
{NodeID: 10, Meta: Meta{Time: 1, Items: []KeyValue{}}},
do we actually need to specify these empty slices? If yes, it might be worth changing the handling code instead so that it doesn't differentiate between empty or nil slice.
Well, nil and empty slice are different values actually. When writing tests, a bug was found (and fixed): #507 (comment)
So we expect now that it will be empty slices if not metadata exists.
Normally this is a bit of an anti-pattern: https://google.github.io/styleguide/go/decisions#nil-slices. e.g. in this case, when no metadata exists, there's no real difference between empty slice or nil slice. If the entity containing the metadata doesn't exist in the first place, that can be handled with an
error
.@ -57,1 +57,4 @@
}
type NodeInfo struct {
NodeID Node
stutters a bit. Just
ID
?fixed
@ -122,0 +152,4 @@
acc := subTreeAcc{errIndex: -1}
err := getSubTree(context.Background(), &acc, d.CID, &GetSubTreeRequest_Body{
TreeId: treeID,
RootId: 0,
no need to specify zero values (
RootId
andDepth
)fixed
@ -485,0 +493,4 @@
return nodes
}
less := func(i, j int) bool {
mm, this might break if we add more sorting criteria later (i.e. more values of
Order_Direction
). Maybe add an explicitswitch
?fixed
8e92e46463
to613ab8d90b
@ -971,0 +971,4 @@
childInfo := NodeInfo{
ID: childID,
}
parentID, _, metaBytes, found := t.getState(b, stateKey(key, childID))
Hm, so we cannot possibly do any paging there if we wanted to? (receive the first 100 items from the db). Do attributes stored by
internalKey
help here?internalKey
has definition// 'i' + attribute name (string) + attribute value (string) + parent (id) + node (id) -> 0/1.
To seek by this index we need to have attribute value.
@ -971,0 +972,4 @@
ID: childID,
}
parentID, _, metaBytes, found := t.getState(b, stateKey(key, childID))
if found {
When can this
found
be false? Isn't it an error?Perhaps. But in the previous implementation, this was not considered an error.
613ab8d90b
tob1342f1ce2
@ -483,2 +492,4 @@
}
func sortByFilename(nodes []pilorama.NodeInfo, d GetSubTreeRequest_Body_Order_Direction) ([]pilorama.NodeInfo, error) {
if len(nodes) == 0 {
[Optional]
It's really not big deal but from my point view this would be better like that:
isn't this swallowing the error about invalid order direction when there are no nodes?
you are right, fixed
b1342f1ce2
toeed594431f