TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 6 Projects Releases 1 Activity Actions

Drop/resolve TODO's #847

Closed
dstepanov-yadro wants to merge 1 commit from dstepanov-yadro/frostfs-node:fix/todo into master
pull from: dstepanov-yadro/frostfs-node:fix/todo
merge into: TrueCloudLab:master
Conversation 1 Commits 1 Files changed 30 +38 -65
dstepanov-yadro commented 2023-12-06 13:29:17 +00:00
Member
Copy link

The most of TODOs are more than one year old, so looks like they will be never resolved.

Some "young" TODOs are still present.

The most of TODOs are more than one year old, so looks like they will be never resolved. Some "young" TODOs are still present.
dstepanov-yadro force-pushed fix/todo from 48efdb391a to 940554edce 2023-12-06 13:30:44 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2023-12-06 13:31:04 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-12-06 13:31:09 +00:00
dstepanov-yadro force-pushed fix/todo from 940554edce to 63a87eb3c5 2023-12-06 13:32:10 +00:00 Compare
dstepanov-yadro force-pushed fix/todo from 63a87eb3c5 to d4be3ea940 2023-12-06 13:32:55 +00:00 Compare
fyrchik reviewed 2023-12-06 13:35:42 +00:00
pkg/local_object_storage/engine/evacuate.go Outdated
@ -276,4 +276,2 @@
for {
listPrm.WithCursor(c)
// TODO (@fyrchik): #1731 this approach doesn't work in degraded modes
fyrchik commented 2023-12-06 13:34:30 +00:00
Owner
Copy link

Please, let's leave this TODO as it is, it is sth we would like to support.

Please, let's leave this TODO as it is, it is sth we would like to support.
dstepanov-yadro commented 2023-12-06 13:47:08 +00:00
Author
Member
Copy link

#848

https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/848
fyrchik commented 2023-12-06 16:00:47 +00:00
Owner
Copy link

What is the problem with still having it in code?

What is the problem with still having it in code?
dstepanov-yadro commented 2023-12-07 06:02:51 +00:00
Author
Member
Copy link

The task can be scheduled and completed. The comment in the code will be lost.

The task can be scheduled and completed. The comment in the code will be lost.
fyrchik commented 2023-12-07 06:59:07 +00:00
Owner
Copy link

The comment reminds you that the task exists in the first place, so everyone who touches the code sees possible implications.

The comment reminds you that the task exists in the first place, so everyone who touches the code sees possible implications.
dstepanov-yadro commented 2023-12-07 08:43:21 +00:00
Author
Member
Copy link

Ok, reverted comment.

Ok, reverted comment.
fyrchik commented 2023-12-07 11:25:44 +00:00
Owner
Copy link

It is different now and misses the link to the task.

It is different now and misses the link to the task.
dstepanov-yadro commented 2023-12-07 12:59:54 +00:00
Author
Member
Copy link

The meaning of the comment is the same. Why store references to tasks in the code?

The meaning of the comment is the same. Why store references to tasks in the code?
fyrchik commented 2023-12-07 14:23:50 +00:00
Owner
Copy link

To link to the task and discussion (and not to create it again).

To link to the task and discussion (and not to create it again).
dstepanov-yadro commented 2023-12-07 17:11:28 +00:00
Author
Member
Copy link

Source code is not the best place for discussions:)

Then we need to add links to all tasks from gitea to the source code.

Source code is not the best place for discussions:) Then we need to add links to all tasks from gitea to the source code.
fyrchik commented 2023-12-08 07:36:13 +00:00
Owner
Copy link

Source code is not the best place for discussions:)

Exactly, and link links to a proper place for discussion.

Then we need to add links to all tasks from gitea to the source code.

No, we don't. In some cases we feel the need to leave a comment, nothing wrong with it.

>Source code is not the best place for discussions:) Exactly, and link links to a proper place for discussion. >Then we need to add links to all tasks from gitea to the source code. No, we don't. In _some_ cases we feel the need to leave a comment, nothing wrong with it.
pkg/services/object/get/container.go Outdated
@ -79,4 +79,2 @@
// TODO: #1142 consider parallel execution
// TODO: #1142 consider optimization: if status == SPLIT we can continue until
// we reach the best result - split info with linking object ID.
fyrchik commented 2023-12-06 13:35:15 +00:00
Owner
Copy link

This is a continuation of TODO comment, could remove

This is a continuation of `TODO` comment, could remove
dstepanov-yadro commented 2023-12-06 13:47:59 +00:00
Author
Member
Copy link

done

done
dstepanov-yadro force-pushed fix/todo from d4be3ea940 to 41e22f69e7 2023-12-06 13:47:54 +00:00 Compare
acid-ant reviewed 2023-12-07 06:50:02 +00:00
cmd/frostfs-cli/modules/object/get.go Outdated
@ -108,7 +108,6 @@ func getObject(cmd *cobra.Command, _ []string) {
func processResult(cmd *cobra.Command, res *internalclient.GetObjectRes, binary bool, payloadBuffer *bytes.Buffer, out io.Writer, filename string) {
if binary {
objToStore := res.Header()
// TODO(@acid-ant): #1932 Use streams to marshal/unmarshal payload
acid-ant commented 2023-12-07 06:47:21 +00:00
Member
Copy link

Could you please replace it on #849?

Could you please replace it on #849?
dstepanov-yadro commented 2023-12-07 08:44:51 +00:00
Author
Member
Copy link

Why do I need a link to the task in the code? I think it is possible to delete this TODO, especially if there is a task.

Why do I need a link to the task in the code? I think it is possible to delete this TODO, especially if there is a task.
👍 1
acid-ant commented 2023-12-07 09:17:17 +00:00
Member
Copy link

Yeah, agree.

Yeah, agree.
cmd/frostfs-cli/modules/object/put.go Outdated
@ -156,7 +156,6 @@ func readFilePayload(filename string, cmd *cobra.Command) (io.Reader, cid.ID, us
buf, err := os.ReadFile(filename)
commonCmd.ExitOnErr(cmd, "unable to read given file: %w", err)
objTemp := objectSDK.New()
// TODO(@acid-ant): #1932 Use streams to marshal/unmarshal payload
acid-ant commented 2023-12-07 06:47:30 +00:00
Member
Copy link

Could you please replace it on #849?

Could you please replace it on #849?
dstepanov-yadro force-pushed fix/todo from 41e22f69e7 to cb6d097e0a 2023-12-07 08:42:59 +00:00 Compare
dstepanov-yadro commented 2023-12-08 11:13:19 +00:00
Author
Member
Copy link

My position on TODO has not found support.

My position on TODO has not found support.
dstepanov-yadro closed this pull request 2023-12-08 11:13:20 +00:00
All checks were successful
DCO action / DCO (pull_request) Successful in 2m7s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m21s
Required
Details
Build / Build Components (1.20) (pull_request) Successful in 3m43s
Required
Details
Build / Build Components (1.21) (pull_request) Successful in 3m38s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 5m59s
Required
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 6m26s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 6m38s
Required
Details
Tests and linters / Lint (pull_request) Successful in 6m58s
Required
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 3m8s
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
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
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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 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-node#847
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/todo"

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?