pkg: remove old duplicated task #8234
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
bot
added
size/M
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8234 +/- ##
==========================================
+ Coverage 77.28% 77.36% +0.07%
==========================================
Files 471 471
Lines 61383 61395 +12
==========================================
+ Hits 47441 47497 +56
+ Misses 10382 10338 -44
Partials 3560 3560
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
bot
added
size/L
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
pkg/ratelimit/runner.go
Outdated
| @@ -42,16 +42,16 @@ const ( | |||
|
|
|||
| // Runner is the interface for running tasks. | |||
| type Runner interface { | |||
| RunTask(ctx context.Context, name string, f func(context.Context), opts ...TaskOption) error | |||
| RunTask(id, name string, f func(), opts ...TaskOption) error | |||
There was a problem hiding this comment.
I think RunTask(id uint64, name string, f func(), opts ...TaskOption) error is ok. fmt.Sprint has an allocation cost.
There was a problem hiding this comment.
But the same id may have different tasks. If we don't use fmt.Sprint, then we need a map[uint64]map[string]struct{}.
There was a problem hiding this comment.
package main
import (
"fmt"
"strings"
"testing"
)
func BenchmarkSprintfKey(b *testing.B) {
m := make(map[string]int)
b.ReportAllocs()
for i := 0; i < b.N; i++ {
key := fmt.Sprintf("%s %s", "hello", "world")
m[key]++
}
}
func BenchmarkStructKey(b *testing.B) {
type myKey struct{
id int64
name string
}
m := make(map[myKey]int)
b.ReportAllocs()
for i := 0; i < b.N; i++ {
key := myKey{id: 1, name: "test"}
m[key]++
}
}
➜ bench_sprintf go test -bench=. -benchmem
goos: darwin
goarch: arm64
pkg: bench
BenchmarkSprintfKey-12 20207793 60.09 ns/op 16 B/op 1 allocs/op
BenchmarkStructKey-12 83140879 14.91 ns/op 0 B/op 0 allocs/op
| @@ -188,22 +193,25 @@ func (cr *ConcurrentRunner) RunTask(ctx context.Context, name string, f func(con | |||
|
|
|||
| pendingTaskNum := len(cr.pendingTasks) | |||
| if pendingTaskNum > 0 { | |||
| if t, ok := cr.pendingRegionTasks[task.id]; ok { | |||
| t.f = f | |||
There was a problem hiding this comment.
Can you add a comment to explain the function will be overwrite and it doesn't matter
rest lgtm
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@okJiang: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
| Start() | ||
| Stop() | ||
| } | ||
|
|
||
| // Task is a task to be run. | ||
| type Task struct { | ||
| ctx context.Context | ||
| id uint64 |
There was a problem hiding this comment.
I think it's ok and use less memory.
pkg/ratelimit/runner.go
Outdated
| @@ -61,16 +61,23 @@ type Task struct { | |||
| var ErrMaxWaitingTasksExceeded = errors.New("max waiting tasks exceeded") | |||
|
|
|||
| // ConcurrentRunner is a simple task runner that limits the number of concurrent tasks. | |||
There was a problem hiding this comment.
It's a mismatched comment line.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato, nolouch, okJiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/merge |
|
@rleungx: We have migrated to builtin 👉 Please use
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
rleungx
added
require-LGT1
|
@rleungx: Your PR was out of date, I have automatically updated it for you. If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: ref #7897.
What is changed and how does it work?
Check List
Tests
Release note