container: Add ListStream method #1453
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
6 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1453
Loading…
Reference in a new issue
No description provided.
Delete branch "elebedeva/frostfs-node:feat/stream-for-list"
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?
#1452
Before:
After:
Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com
989336df37
tod816e508ab
d816e508ab
tofe3225bbae
WIP: container: Add ListStream methodto container: Add ListStream methodfe3225bbae
tof24b837331
f24b837331
to9439cee550
9439cee550
toc66e3b11a2
Will remove module replacement in
go.mod
after TrueCloudLab/frostfs-sdk-go#291 is merged.@ -88,0 +119,4 @@
for {
n, ok = rdr.Read(buf)
for i := range n {
list = append(list, buf[i])
How about to use callback here instead of append?
Done
@ -88,0 +132,4 @@
}
sort.Slice(list, func(i, j int) bool {
lhs, rhs := list[i].EncodeToString(), list[j].EncodeToString()
Can we compare IDs using their byte representation instead of as strings?
I think, it's better to use
slices.SortFunc
instead ofsort.Slice
according to15102e6dfd
Replaced
sort.Slice
withslices.SortFunc
.About comparing IDs as byte arrays: we do have
cid.ID.Encode(dst []byte)
, but IMO string comparison is easier to read.@a-savchuk we can after TrueCloudLab/frostfs-sdk-go#294
@ -842,2 +843,4 @@
fatalOnErr(err)
amount := config.UintSafe(appCfg.Sub("morph"), "container_batch_size")
if amount <= 0 {
It can't be less than zero,
if amount == 0
is enoughRemoved this entire section as you suggested here.
@ -843,1 +844,4 @@
amount := config.UintSafe(appCfg.Sub("morph"), "container_batch_size")
if amount <= 0 {
amount = 1000
How about reading this value in a separate function in
cmd/frostfs-node/config/morph.go
, similar to how it's done forcontainer_cache_size
(examples below)? You can also define the default value as a constant there// ContainerCacheSizeDefault represents the default size for the container cache.
ContainerCacheSizeDefault = 100
// ContainerCacheSize returns the value of "container_cache_size" config parameter
// from "morph" section.
//
// Returns 0 if the value is not positive integer.
// Returns ContainerCacheSizeDefault if the value is missing.
func ContainerCacheSize(c *config.Config) uint32 {
if c.Sub(subsection).Value("container_cache_size") == nil {
return ContainerCacheSizeDefault
}
return config.Uint32Safe(c.Sub(subsection), "container_cache_size")
}
What do you think?
Moreover, I think you should write a test for reading this new configuration option. You can find an example in cmd/frostfs-node/config/morph/config_test.go.
This test will read the new option from the config file in different formats, so you may need to replicate this option in those formats as mentioned in one of my comments:
From your recent commit
591479e92b
:I think, we expect
container_batch_size
either to be set in config or have the default value; zero batch size has no sense. What about that?Done!
@ -88,0 +91,4 @@
}
// SortedIDList returns sorted identifiers of the matched containers.
func (x ContainerListStreamRes) SortedIDList() []cid.ID {
I didn't quite catch why we need to sort it here. I see that we already sort it at the end of
ListContainersStream
. Could you please explain your idea?Oh, I forgot to remove sorting from
internal.ListContainersStream()
. The idea was that sorting ofContainerListStreamRes
looks similar toContainerListRes
. Now it's irrelevant though, asinternal.ListContainersStream()
returns only error and printscid
directly tocmd
.Thank you. Do we still need
ContainerListStreamRes
structure?No, we don't. Removed
@ -83,6 +83,7 @@ morph:
# Default value: block time. It is recommended to have this value less or equal to block time.
# Cached entities: containers, container lists, eACL tables.
container_cache_size: 100 # container_cache_size is is the maximum number of containers in the cache.
container_batch_size: 1000 # container_batch_size is the maximum amount of containers to send via stream at once
We have configuration examples in YAML, JSON, and dotenv formats, which mostly duplicate each other. It'd be very helpful if you could replicate this new configuration option in the other config files we have
Fixed
c66e3b11a2
to9a61583e3a
9a61583e3a
to360d230c09
@ -58,0 +62,4 @@
if e, ok := status.FromError(err); ok {
switch e.Code() {
case codes.Unimplemented:
resV1, err := internalclient.ListContainers(cmd.Context(), prm)
Could you, please, use another naming for the variable? I mean,
...V1
suffix seems inconvenient as it may be mistaken for old api-go version.resNonStream
could be absolutely fineRenamed it to just
res
asinternal.ListContainersStream()
returns only error now.@ -58,0 +59,4 @@
var containerIDs []cid.ID
res, err := internalclient.ListContainersStream(cmd.Context(), prm)
if err != nil {
if e, ok := status.FromError(err); ok {
How about
instead of
Fixed
@ -203,0 +230,4 @@
r := new(container.ListStreamResponse)
r.SetBody(resBody)
return stream.Send(r)
Could you explain, please, what is difference between
ListStream
and List then?The response is being sent via
stream
and it seems it sends the only message. I don't see any point to use streaming then.I assumed we would fetch containers by batches and send them via stream - apparently, I am missing something
Nevermind. I thought the problem is that we can't receive the entire container list on the server side (kind of OOM or something) and I expected we'd receive containers by batches and stream these batches to the client.
I glanced at the issue - the problem is basically with
List
handler that causes the error for the client side. So,stream.Send
gracefully handles this 👍360d230c09
to12d8706da2
12d8706da2
to591479e92b
@ -88,0 +92,4 @@
// SortedIDList returns sorted identifiers of the matched containers.
func (x ContainerListStreamRes) SortedIDList() []cid.ID {
list := x.ids
What do you achieve by introducing a variable
list
?This function is removed as it is no longer needed.
@ -88,0 +110,4 @@
return res, fmt.Errorf("init container list: %w", err)
}
buf := make([]cid.ID, 10)
What is 10?
It was hardcoded buffer size. Doesn't matter anymore since
rdr.Read()
that needed buffer to read to was replaced withrdr.Iterate()
.@ -88,0 +117,4 @@
var ok bool
for {
n, ok = rdr.Read(buf)
Why not reuse
rdr.Iterate(...)
?Replaced
rdr.Read()
withrdr.Iterate()
.@ -52,3 +55,3 @@
var prm internalclient.ListContainersPrm
prm.SetClient(cli)
prm.Account = idUser
prm.OwnerID = idUser
This change should be in a separate commit (unless there are some problems with this).
There is an issue with moving this one change to a separate commit: in TrueCloudLab/frostfs-sdk-go#291 this field was renamed after
ListStream
was added.I can add a line about this change in the commit description.
The mere presense of the
ListStream
in the SDK should affect nothing, right?The problem is that server interface needs to be implemented, thats why
@ -839,9 +840,15 @@ func initContainer(appCfg *config.Config) cfgContainer {
containerWorkerPool, err := ants.NewPool(notificationHandlerPoolSize)
fatalOnErr(err)
amount := config.UintSafe(appCfg.Sub("morph"), "container_batch_size")
Why have you chosen
morph
section?We have
object
section whereput
andget
section may be configured.SImilarly we could have
container
section withlist_stream
configuration.Moved from
morph
tocontainer
.@ -50,6 +50,7 @@ func (c *cfg) initMorphComponents(ctx context.Context) {
var netmapSource netmap.Source
c.cfgMorph.containerCacheSize = morphconfig.ContainerCacheSize(c.appCfg)
c.cfgMorph.containerBatchSize = morphconfig.ContainerBatchSize(c.appCfg)
Again, why is it in
cfgMorph
?591479e92b
tobd4c4828f0
bd4c4828f0
to64b9bf1d7f
64b9bf1d7f
to75ac7278dd
75ac7278dd
to5be8806e4a
5be8806e4a
tod477a4acf1
d477a4acf1
to2bd0ab4cb8
@ -88,0 +95,4 @@
}
if rdr.Iterate(printCnr) != nil {
return fmt.Errorf("read container list: %w", err)
It seems that
err
will always benil
here.You're right, fixed
2bd0ab4cb8
to7ffc82a066
7ffc82a066
to03d5a1d5ad
03d5a1d5ad
toec0fc52e56
LGTM
@ -85,3 +83,4 @@
return list
}
func ListContainersStream(ctx context.Context, prm ListContainersPrm, printCnr func(id cid.ID) bool) (err error) {
In common,
printCnr
can do anything, not onlyprint
.Renamed to
processCnr
.@ -62,0 +61,4 @@
var containerIDs []cid.ID
err := internalclient.ListContainersStream(cmd.Context(), prm, func(id cid.ID) bool {
if flagVarListName == "" && !flagVarListPrintAttr {
It looks like the code below for "normal" listing.
Could you move it to a separate function? It may be a lambda defined locally.
But both printing routines are the same, I see no situation where one would be changed without the other.
Another approach would be to reuse loop and define iterator over the slice.
Moved container printing to a separate function.
ec0fc52e56
toc1627c8eac
New commits pushed, approval review dismissed automatically according to repository settings
c1627c8eac
toe9971d73c8
@ -84,2 +84,4 @@
FROSTFS_REPLICATOR_POOL_SIZE=10
# Container service section
FROSTFS_CONTAINER_LIST_STREAM_BATCH_SIZE=1000
The value you use in the example is the default. The test is somewhat useless then.
I suggest using some other value, e.g. 500.
Fixed.
Tests are failing
Forgot to change the value in
TestContainerSection
, should be fine now.e9971d73c8
to06be16b716
06be16b716
todf05057ed4
Very nice! 👍
Exquisite! 🥇