TrueCloudLab/frostfs-sdk-go
33
0
Fork 21
Code Issues 34 Pull requests 1 Releases Activity Actions

container: Add ListStream method #291

Merged
fyrchik merged 3 commits from elebedeva/frostfs-sdk-go:feat/stream-for-list into master 2024-12-10 11:11:41 +00:00
Conversation 0 Commits 3 Files changed 15 +653 -6
elebedeva commented 2024-10-28 15:21:28 +00:00
Member
Copy link

TrueCloudLab/frostfs-node#1452

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

TrueCloudLab/frostfs-node#1452 Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva added 1 commit 2024-10-28 15:21:29 +00:00
[#XX] container: Add ListStream method
Some checks failed
DCO / DCO (pull_request) Failing after 1m49s
Details
Tests and linters / Tests (pull_request) Failing after 1m47s
Details
Tests and linters / Lint (pull_request) Failing after 2m21s
Details
d8f1cc29f5 
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva force-pushed feat/stream-for-list from d8f1cc29f5 to f85d218adc 2024-11-15 13:30:50 +00:00 Compare
elebedeva force-pushed feat/stream-for-list from f85d218adc to bbf3cb9478 2024-11-26 15:01:58 +00:00 Compare
elebedeva force-pushed feat/stream-for-list from bbf3cb9478 to ce54462437 2024-11-27 11:47:26 +00:00 Compare
elebedeva changed title from WIP: container: Add ListStream method to container: Add ListStream method 2024-11-27 11:58:49 +00:00
elebedeva requested review from storage-core-committers 2024-11-27 17:21:04 +00:00
elebedeva requested review from storage-core-developers 2024-11-27 17:21:05 +00:00
aarifullin reviewed 2024-11-28 12:32:50 +00:00
client/container_list_stream.go Outdated
@ -0,0 +31,4 @@
// Required parameter.
//
// Deprecated: Use PrmContainerListStream.Account instead.
func (x *PrmContainerListStream) SetAccount(id user.ID) {
aarifullin commented 2024-11-28 12:32:50 +00:00
Member
Copy link

You can't get deprecated the method that has never been used as it's new :) So, you don't need this setter

You can't get deprecated the method that has never been used as it's new :) So, you don't need this setter
👍 2
elebedeva commented 2024-12-06 14:17:11 +00:00
Author
Member
Copy link

Removed! 🙈

Removed! 🙈
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-11-28 13:30:52 +00:00
client/container_list_stream.go Outdated
@ -0,0 +79,4 @@
func (x *ContainerListReader) Read(buf []cid.ID) (int, bool) {
if len(buf) == 0 {
panic("empty buffer in ContainerListReader.ReadList")
aarifullin commented 2024-11-28 13:30:52 +00:00
Member
Copy link

Read instead of Read.List ? :)
To be honest, this panic looks unnecessarily. You can define

var errEmptyBuffer = errors.New("empty read buffer")

if len(buf) == 0 {
   x.err = errEmptyBuffer
   return 0, false
}
`Read` instead of `Read.List` ? :) To be honest, this panic looks unnecessarily. You can define ```go var errEmptyBuffer = errors.New("empty read buffer") ``` ```go if len(buf) == 0 { x.err = errEmptyBuffer return 0, false } ```
fyrchik commented 2024-11-30 04:14:07 +00:00
Owner
Copy link

Why do we panic or error at all?
If we remove this if, will the code still work?

Why do we panic or error at all? If we remove this `if`, will the code still work?
elebedeva commented 2024-12-10 09:56:25 +00:00
Author
Member
Copy link

If we remove this check the list command will hang indefinitely. I believe we should keep the panic as the clear indication of incorrect passed argument.

If we remove this check the list command will hang indefinitely. I believe we should keep the panic as the clear indication of incorrect passed argument.
aarifullin reviewed 2024-11-28 13:37:32 +00:00
client/container_list_stream.go Outdated
@ -0,0 +113,4 @@
if read == len(buf) {
// save the tail
x.tail = append(x.tail, ids[ln:]...)
aarifullin commented 2024-11-28 13:37:32 +00:00
Member
Copy link

Optional change

That's barely possible for listing containers but if read buffer is quite small and stream is going to send a big number of containers then such accumulation in the tail may lead to overflow.
Didn't you consider?

if len(x.tail)+len(remainingIDs) > x.maxTailSize {
   x.err = errors.New("internal buffer overflow")
   return 0, false
}

P.S. about small buffer: Iterate already uses such buffer

### Optional change That's barely possible for listing containers but if read buffer is quite small and stream is going to send a big number of containers then such accumulation in the tail may lead to overflow. Didn't you consider? ```go if len(x.tail)+len(remainingIDs) > x.maxTailSize { x.err = errors.New("internal buffer overflow") return 0, false } ``` P.S. about small buffer: `Iterate` already uses such buffer
elebedeva commented 2024-12-10 09:23:50 +00:00
Author
Member
Copy link

I don't see how accumulation in the tail may lead to overflow. We do exactly the same in Object.Search here, and it seems there's been no problem so far.

I don't see how accumulation in the tail may lead to overflow. We do exactly the same in `Object.Search` [here](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/81c423e7094d870f7e38e5ccca8e4690dfe89699/client/object_search.go#L167), and it seems there's been no problem so far.
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-11-28 13:41:21 +00:00
client/container_list_stream.go Outdated
@ -0,0 +22,4 @@
type PrmContainerListStream struct {
XHeaders []string
Account user.ID
aarifullin commented 2024-11-28 13:41:21 +00:00
Member
Copy link

Please, leave a comment how this parameter is used :)

Please, leave a comment how this parameter is used :)
elebedeva commented 2024-12-10 09:26:48 +00:00
Author
Member
Copy link

For request building, as in existing Container.List

For request building, as in existing `Container.List`
aarifullin commented 2024-12-10 10:11:02 +00:00
Member
Copy link

I meant - to leave a comment in the code :) to describe the field. But as far as I see you fixed the field's name. So, the comment is not required

I meant - to leave a comment in the code :) to describe the field. But as far as I see you fixed the field's name. So, the comment is not required
👍 1
dstepanov-yadro reviewed 2024-11-29 13:50:43 +00:00
client/container_list_stream.go Outdated
@ -0,0 +23,4 @@
XHeaders []string
Account user.ID
dstepanov-yadro commented 2024-11-29 13:50:43 +00:00
Member
Copy link

Maybe Account -> OwnerID?

Maybe `Account` -> `OwnerID`?
👍 2
fyrchik commented 2024-12-09 06:52:52 +00:00
Owner
Copy link

It should be the same as PrmContainerList.
Please, either use Account or rename Account to OwnerID in PrmContainerList (in a separate commit, of course).

It should be the same as `PrmContainerList`. Please, either use `Account` or rename `Account` to `OwnerID` in `PrmContainerList` (in a separate commit, of course).
elebedeva commented 2024-12-10 09:58:24 +00:00
Author
Member
Copy link

Fixed

Fixed
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2024-11-29 13:58:56 +00:00
client/container_list_stream.go Outdated
@ -0,0 +77,4 @@
tail []refs.ContainerID
}
func (x *ContainerListReader) Read(buf []cid.ID) (int, bool) {
dstepanov-yadro commented 2024-11-29 13:58:56 +00:00
Member
Copy link

int is count of read containers. What is bool?

`int` is count of read containers. What is `bool`?
elebedeva commented 2024-12-06 14:18:03 +00:00
Author
Member
Copy link

bool indicates if Read() was successful or not.

`bool` indicates if `Read()` was successful or not.
elebedeva force-pushed feat/stream-for-list from ce54462437 to cdc94cf668 2024-12-06 14:04:35 +00:00 Compare
fyrchik approved these changes 2024-12-09 06:51:45 +00:00
Dismissed
api/rpc/container.go Outdated
@ -83,0 +96,4 @@
req *container.ListStreamRequest,
opts ...client.CallOption,
) (*ListStreamResponseReader, error) {
wc, err := client.OpenServerStream(cli, common.CallMethodInfoServerStream(serviceContainer, rpcContainerStream), req, opts...)
fyrchik commented 2024-12-09 06:48:15 +00:00
Owner
Copy link

This line is also changed in the second commit, it shouldn't be there.

This line is also changed in the second commit, it shouldn't be there.
elebedeva commented 2024-12-10 09:27:31 +00:00
Author
Member
Copy link

Fixed

Fixed
elebedeva force-pushed feat/stream-for-list from cdc94cf668 to 83b26b08b9 2024-12-10 09:13:05 +00:00 Compare
elebedeva added 1 commit 2024-12-10 09:19:36 +00:00
[#291] container: Rename field Account to OwnerID in PrmContainerList
All checks were successful
DCO / DCO (pull_request) Successful in 1m23s
Details
Tests and linters / Tests (pull_request) Successful in 1m44s
Details
Tests and linters / Lint (pull_request) Successful in 2m9s
Details
ff8ef25438 
Renaming this field so that it matches PrmContainerListStream.

Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva dismissed fyrchik's review 2024-12-10 09:19:37 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

elebedeva force-pushed feat/stream-for-list from ff8ef25438 to 5e7bc0588e 2024-12-10 09:25:02 +00:00 Compare
aarifullin approved these changes 2024-12-10 10:11:29 +00:00
Dismissed
aarifullin left a comment
Member
Copy link

LGTM

LGTM
fyrchik approved these changes 2024-12-10 10:19:59 +00:00
Dismissed
dstepanov-yadro approved these changes 2024-12-10 10:42:16 +00:00
Dismissed
fyrchik reviewed 2024-12-10 10:46:10 +00:00
pool/pool.go Outdated
@ -2887,6 +2959,22 @@ func (p *Pool) ListContainers(ctx context.Context, prm PrmContainerList) ([]cid.
return cnrIDs, nil
}
// // ListContainers requests identifiers of the account-owned containers.
fyrchik commented 2024-12-10 10:46:10 +00:00
Owner
Copy link

Please, remove one //

Please, remove one `//`
fyrchik commented 2024-12-10 10:46:42 +00:00
Owner
Copy link

Also, fix the name.

Also, fix the name.
elebedeva commented 2024-12-10 10:50:53 +00:00
Author
Member
Copy link

Fixed

Fixed
elebedeva force-pushed feat/stream-for-list from 5e7bc0588e to c4463df8d4 2024-12-10 10:50:13 +00:00 Compare
elebedeva dismissed aarifullin's review 2024-12-10 10:50:13 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

elebedeva dismissed fyrchik's review 2024-12-10 10:50:13 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

elebedeva dismissed dstepanov-yadro's review 2024-12-10 10:50:13 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

fyrchik approved these changes 2024-12-10 11:11:37 +00:00
fyrchik merged commit c4463df8d4 into master 2024-12-10 11:11:41 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
pool

Tasks related to client pool

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
pool
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-sdk-go#291
No description provided.
Delete branch "elebedeva/frostfs-sdk-go: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?