Conversation
aiivashchenko
requested review from
arttor,
fa-at-pulsit and
schucly
as code owners
aiivashchenko
force-pushed
the
diff-fix
branch
9 times, most recently
from
cee5861 to
14d7444
Compare
pkg/objstore/service.go
Outdated
| ObjectExists(ctx context.Context, bucket string, name string, opts ...func(o *commonObjectOptions)) (bool, error) | ||
| RemoveObject(ctx context.Context, bucket string, name string, opts ...func(o *commonObjectOptions)) error | ||
| RemoveObjects(ctx context.Context, bucket string, names []string) error | ||
| RemoveObjects(ctx context.Context, bucket string, names []string) iter.Seq[error] |
There was a problem hiding this comment.
why removeObjects returns iterator here? It accepts fixed list of objects without streaming/paging and the same is true for response?
- S3 returns list in a single response https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html#API_DeleteObjects_ResponseSyntax
- swift bulk delete also https://docs.openstack.org/swift/latest/api/bulk-delete.html
There was a problem hiding this comment.
Do you mean you'd prefer to have a collection instead of iterator?
One of the reasons the iterator is here is because one of the interfaces returns either a channel or iterator.
It can be precollected, of course.
But in that case, we'd do extra loop on errors.
Not a big deal, but still.
There was a problem hiding this comment.
yes collection is better. slice of objects names IN -> slice of object erros OUT - very simple.
Iterators are not obvious pattern for me here. i would expect it only in API with paging/cursors.
There was a problem hiding this comment.
Done on both, RemoveObjects and RemoveObjectVersions
| SetReplicationID(id entity.UniversalReplicationID) | ||
| GetReplicationID() entity.UniversalReplicationID | ||
| SetInheritableQueue(queue string) | ||
| GetInheritableQueue() string |
There was a problem hiding this comment.
what do you think about explicitQueue or queueOverride?
There was a problem hiding this comment.
I don't have a strong opinion on naming.
But queue is set to parent task, that should produce other tasks with this queue.
So, for parent task, we are not overriding anything, or setting anything explicit.
Therefor, I thought that inheritableQueue is a more transparent name.
There was a problem hiding this comment.
I would name it OverrideQueue or QueueOverride because by default all tasks have some queue assigned based on its type and here we can override default value.
Signed-off-by: Andrei Ivashchenko <aiivashchenko@users.noreply.github.com>
2 participants
Pull Request
Closes #145
Checklist
Signed-off-byline to certify agreement with the Developer Certificate of Origin (DCO).make testpassingNote: By submitting this PR, you agree to license your contributions under the Apache 2.0 License and follow our Code of Conduct.