You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/11/12 18:30:23 UTC
[GitHub] [skywalking-banyandb] mikechengwei opened a new pull request, #214: Fix stream's leaked goroutines
mikechengwei opened a new pull request, #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214
Fix stream's leaked goroutines.
Signed-off-by: mikechengwei <84...@qq.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313211926
/run-all-tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1020920774
##########
banyand/metadata/schema/etcd.go:
##########
@@ -83,6 +83,7 @@ func (eh *eventHandler) InterestOf(kind Kind) bool {
type etcdSchemaRegistry struct {
server *embed.Etcd
+ client *clientv3.Client
Review Comment:
Because etcd client init at
https://github.com/apache/skywalking-banyandb/blob/c1302224dcb7e4113b0aef59d9005dd38f1eee30/banyand/metadata/schema/etcd.go#L168
I need to convert the temporary variable to a property of `etcdSchemaRegistry `so that I can close the client resource.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313161529
@mikechengwei Please use `make format` to reorder the imports
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313163922
> @mikechengwei Please use `make format` to reorder the imports
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1020837387
##########
banyand/metadata/schema/etcd.go:
##########
@@ -83,6 +83,7 @@ func (eh *eventHandler) InterestOf(kind Kind) bool {
type etcdSchemaRegistry struct {
server *embed.Etcd
+ client *clientv3.Client
Review Comment:
Why do you add a client here since there are no references to it?
##########
banyand/tsdb/index/writer.go:
##########
@@ -121,6 +121,7 @@ func (s *Writer) Write(value Message) {
}
func (s *Writer) Close() error {
+ close(s.ch)
Review Comment:
That might raise a data race. You should leverage `run.Closer` to notify senders to stop sending data:
```go
func (s *Writer) Write(value Message) {
if !s.closer.AddRunning() {
return
}
defer s.closer.Done()
go func(m Message) {
select {
case <-s.closer.CloseNotify():
return
case s.ch <- m:
}
}(value)
}
func (s *Writer) Close() error {
s.closer.CloseThenWait()
close(s.ch)
return nil
}
```
##########
banyand/queue/local.go:
##########
@@ -32,6 +32,10 @@ type local struct {
repo discovery.ServiceRepo
}
+func (l *local) Close() {
+ l.local.Close()
+}
+
Review Comment:
```suggestion
// GracefulStop implements Queue
func (l *local) GracefulStop() {
l.local.Close()
close(l.stopCh)
}
// Serve implements Queue
func (l *local) Serve() run.StopNotify {
return l.stopCh
}
```
##########
banyand/queue/local.go:
##########
@@ -32,6 +32,10 @@ type local struct {
repo discovery.ServiceRepo
Review Comment:
```suggestion
repo discovery.ServiceRepo
stopCh chan struct{}
```
##########
banyand/queue/queue.go:
##########
@@ -30,6 +30,7 @@ type Queue interface {
run.Unit
bus.Subscriber
bus.Publisher
+ Close()
Review Comment:
Since you add `Close`, `Queue` becomes a `run.Service` more than a `run.Unit`.
##########
banyand/metadata/schema/etcd.go:
##########
@@ -143,6 +144,10 @@ func (e *etcdSchemaRegistry) StoppingNotify() <-chan struct{} {
}
func (e *etcdSchemaRegistry) Close() error {
+ err := e.client.Close()
Review Comment:
If you want to ignore an error, simplify it to `_ = e.client.Close()`
##########
banyand/queue/queue.go:
##########
@@ -30,6 +30,7 @@ type Queue interface {
run.Unit
bus.Subscriber
bus.Publisher
+ Close()
Review Comment:
```suggestion
run.Service
```
##########
pkg/bus/bus.go:
##########
@@ -223,3 +223,11 @@ func (b *Bus) Subscribe(topic Topic, listener MessageListener) error {
}(listener, ch)
return nil
}
+
+func (b *Bus) Close() {
+ for _, chs := range b.topics {
+ for _, ch := range chs {
+ close(ch)
Review Comment:
You also need `run.Closer` to notify the senders as `index.writer` does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily merged pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily merged PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313231643
> LGTM. Thank you.
>
> Would you be interested in other goroutines leaked issues listed at [apache/skywalking#9454](https://github.com/apache/skywalking/issues/9454)
Ok，i can finish other similar issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] wu-sheng commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313220693
> Fixes apache/skywalking/issues/9460
Fix keyword only works in PR description, or finally squashed commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1020922977
##########
banyand/tsdb/index/writer.go:
##########
@@ -121,6 +121,7 @@ func (s *Writer) Write(value Message) {
}
func (s *Writer) Close() error {
+ close(s.ch)
Review Comment:
```
if !s.closer.AddRunning() {
return
}
defer s.closer.Done()
```
I can't understand the meaning of this block code, is it okay to initialize closer to 0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313221667
> > Fixes [apache/skywalking/issues/9460](https://github.com/apache/skywalking/issues/9460)
>
> Fix keyword only works in PR description, or finally squashed commit.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] codecov-commenter commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1312610524
# [Codecov](https://codecov.io/gh/apache/skywalking-banyandb/pull/214?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#214](https://codecov.io/gh/apache/skywalking-banyandb/pull/214?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2339486) into [main](https://codecov.io/gh/apache/skywalking-banyandb/commit/a1600927a2eba856cf347cc3cefabe4fa49f1f06?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a160092) will **decrease** coverage by `0.04%`.
> The diff coverage is `27.27%`.
```diff
@@ Coverage Diff @@
## main #214 +/- ##
==========================================
- Coverage 43.83% 43.79% -0.05%
==========================================
Files 87 87
Lines 8579 8590 +11
==========================================
+ Hits 3761 3762 +1
- Misses 4481 4490 +9
- Partials 337 338 +1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking-banyandb/pull/214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/bus/bus.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/214/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2J1cy9idXMuZ28=) | `79.41% <0.00%> (-4.10%)` | :arrow_down: |
| [banyand/metadata/schema/etcd.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/214/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZXRhZGF0YS9zY2hlbWEvZXRjZC5nbw==) | `60.50% <40.00%> (-0.45%)` | :arrow_down: |
| [banyand/stream/service.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/214/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9zdHJlYW0vc2VydmljZS5nbw==) | `66.23% <100.00%> (+0.44%)` | :arrow_up: |
| [banyand/measure/measure.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/214/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZWFzdXJlL21lYXN1cmUuZ28=) | `80.76% <0.00%> (-3.85%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1021011304
##########
pkg/bus/bus.go:
##########
@@ -223,3 +230,11 @@ func (b *Bus) Subscribe(topic Topic, listener MessageListener) error {
}(listener, ch)
return nil
}
+
+func (b *Bus) Close() {
+ for _, chs := range b.topics {
Review Comment:
Add `closer.CloseThenWait()` before closing channels
##########
banyand/metadata/schema/etcd.go:
##########
@@ -143,6 +144,10 @@ func (e *etcdSchemaRegistry) StoppingNotify() <-chan struct{} {
}
func (e *etcdSchemaRegistry) Close() error {
+ err := e.client.Close()
+ if err != nil {
+ return err
+ }
Review Comment:
Rearrange lines in this function:
```go
e.server.Close()
return e.client.Close()
```
##########
pkg/schema/metadata.go:
##########
@@ -530,6 +530,7 @@ func (g *group) close() (err error) {
for _, s := range g.schemaMap {
err = multierr.Append(err, s.Close())
}
+
Review Comment:
Remove this line.
##########
banyand/tsdb/index/writer.go:
##########
@@ -111,16 +113,23 @@ func NewWriter(ctx context.Context, options WriterOptions) *Writer {
}
w.ch = make(chan Message)
w.bootIndexGenerator()
+ w.closer = run.NewCloser(0)
return w
}
func (s *Writer) Write(value Message) {
go func(m Message) {
Review Comment:
The first step of `Write` is to add a running task to `closer` by
```go
if !s.closer.AddRunning() { return }
defer s.closer.Done()
```
##########
pkg/bus/bus.go:
##########
@@ -175,9 +178,13 @@ func (b *Bus) Publish(topic Topic, message ...Message) (Future, error) {
for _, each := range cc {
for _, m := range message {
go func(ch Channel, message Message) {
Review Comment:
Add this piece in the closure:
```go
if !s.closer.AddRunning() { return }
defer s.closer.Done()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#issuecomment-1313218423
Fixes apache/skywalking/issues/9460
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] mikechengwei commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1021021591
##########
pkg/bus/bus.go:
##########
@@ -223,3 +230,11 @@ func (b *Bus) Subscribe(topic Topic, listener MessageListener) error {
}(listener, ch)
return nil
}
+
+func (b *Bus) Close() {
+ for _, chs := range b.topics {
Review Comment:
Get.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1021032561
##########
pkg/bus/bus.go:
##########
@@ -156,6 +159,10 @@ type Event struct {
}
func (b *Bus) Publish(topic Topic, message ...Message) (Future, error) {
+ if !b.closer.AddRunning() {
+ return nil, nil
+ }
+ defer b.closer.Done()
if topic.ID == "" {
Review Comment:
Put them between line 184 and line 185. Each sender is a running task that should be closed gracefully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #214: Fix stream's leaked goroutines
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #214:
URL: https://github.com/apache/skywalking-banyandb/pull/214#discussion_r1021152207
##########
banyand/tsdb/index/writer.go:
##########
@@ -111,16 +113,27 @@ func NewWriter(ctx context.Context, options WriterOptions) *Writer {
}
w.ch = make(chan Message)
w.bootIndexGenerator()
+ w.closer = run.NewCloser(0)
return w
}
func (s *Writer) Write(value Message) {
+ if !s.closer.AddRunning() {
+ return
+ }
+ defer s.closer.Done()
Review Comment:
Move them between line 125 and line 126 to solve the data race.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org