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 2023/01/01 14:40:21 UTC

[GitHub] [skywalking-banyandb] mikechengwei opened a new pull request, #236: Fix liaison/grpc's leaked goroutines

mikechengwei opened a new pull request, #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236

   Fixes https://github.com/apache/skywalking/issues/9462.
   
   Signed-off-by: mikechengwei [842725815@qq.com](mailto:842725815@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 a diff in pull request #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1063033493


##########
banyand/discovery/discovery.go:
##########
@@ -33,6 +33,7 @@ type ServiceRepo interface {
 	run.Unit
 	bus.Subscriber
 	bus.Publisher
+	GracefulStop()

Review Comment:
   ok



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064154975


##########
banyand/measure/measure_suite_test.go:
##########
@@ -102,7 +105,6 @@ func setUp() (*services, func()) {
 	flags = append(flags, "--etcd-listen-client-url="+listenClientURL, "--etcd-listen-peer-url="+listenPeerURL)
 	moduleDeferFunc := test.SetupModules(
 		flags,
-		repo,

Review Comment:
   If it is not removed, the unit test will block at https://github.com/apache/skywalking-banyandb/blob/1c19243df23f8350ea5c1542fd914c49e8698960/pkg/test/setup.go#L49 and cannot be terminated normally due to discovery_mock `gracefulstop` method.
   
   log screensnapt:
   ![image](https://user-images.githubusercontent.com/13257127/211202607-453cb617-5702-4108-8658-a5b825d7b8b2.png)
   



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064065992


##########
banyand/stream/stream_suite_test.go:
##########
@@ -101,7 +104,6 @@ func setUp() (*services, func()) {
 	flags = append(flags, "--etcd-listen-client-url="+listenClientURL, "--etcd-listen-peer-url="+listenPeerURL)
 	moduleDeferFunc := test.SetupModules(
 		flags,
-		repo,

Review Comment:
   You can't remove it.



##########
pkg/test/setup/setup.go:
##########
@@ -91,7 +91,6 @@ func modules(flags []string) func() {
 
 	return test.SetupModules(
 		flags,
-		repo,

Review Comment:
   The order of sequence should be equal to https://github.com/apache/skywalking-banyandb/blob/main/banyand/internal/cmd/standalone.go#L81-L92



##########
banyand/measure/measure_suite_test.go:
##########
@@ -102,7 +105,6 @@ func setUp() (*services, func()) {
 	flags = append(flags, "--etcd-listen-client-url="+listenClientURL, "--etcd-listen-peer-url="+listenPeerURL)
 	moduleDeferFunc := test.SetupModules(
 		flags,
-		repo,

Review Comment:
   If you remove `repo`, `moduleDeferFunc` can not stop it gracefully.



##########
pkg/test/setup/setup.go:
##########
@@ -91,7 +91,6 @@ func modules(flags []string) func() {
 
 	return test.SetupModules(
 		flags,
-		repo,

Review Comment:
   Why do you move `repo` to the last?



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#issuecomment-1374529417

   @hanahmily please help me review it again ,thanks.


-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#issuecomment-1376774300

   I found that it is still a block, I will spend some time to troubleshoot.


-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064253100


##########
banyand/measure/measure_suite_test.go:
##########
@@ -74,6 +74,9 @@ func setUp() (*services, func()) {
 	// Init Discovery
 	repo := discovery.NewMockServiceRepo(ctrl)
 	repo.EXPECT().NodeID().AnyTimes()
+	repo.EXPECT().Name().AnyTimes()
+	repo.EXPECT().Serve().AnyTimes()
+	repo.EXPECT().GracefulStop().AnyTimes()

Review Comment:
   ```suggestion
   	stopCh := make(chan struct{})
   	repo.EXPECT().Serve().Return(stopCh).Times(1)
   	repo.EXPECT().GracefulStop().Do(func() { close(stopCh) }).Times(1)
   ```



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#issuecomment-1368459658

   # [Codecov](https://codecov.io/gh/apache/skywalking-banyandb/pull/236?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 [#236](https://codecov.io/gh/apache/skywalking-banyandb/pull/236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (55732e7) into [main](https://codecov.io/gh/apache/skywalking-banyandb/commit/2434e593c1a5a44b5628581da7d919238c21eb12?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2434e59) will **increase** coverage by `1.13%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@            Coverage Diff             @@
   ##             main     #236      +/-   ##
   ==========================================
   + Coverage   44.04%   45.18%   +1.13%     
   ==========================================
     Files          88       87       -1     
     Lines        9092     8871     -221     
   ==========================================
   + Hits         4005     4008       +3     
   + Misses       4706     4484     -222     
   + Partials      381      379       -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking-banyandb/pull/236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [banyand/measure/encode.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC9tZWFzdXJlL2VuY29kZS5nbw==) | `25.00% <0.00%> (-2.91%)` | :arrow_down: |
   | [banyand/tsdb/block.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC90c2RiL2Jsb2NrLmdv) | `59.90% <0.00%> (-1.64%)` | :arrow_down: |
   | [pkg/index/lsm/lsm.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-cGtnL2luZGV4L2xzbS9sc20uZ28=) | `78.26% <0.00%> (-0.91%)` | :arrow_down: |
   | [pkg/index/lsm/iterator.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-cGtnL2luZGV4L2xzbS9pdGVyYXRvci5nbw==) | `81.89% <0.00%> (-0.46%)` | :arrow_down: |
   | [pkg/encoding/int.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-cGtnL2VuY29kaW5nL2ludC5nbw==) | `87.91% <0.00%> (-0.17%)` | :arrow_down: |
   | [pkg/encoding/xor.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-cGtnL2VuY29kaW5nL3hvci5nbw==) | `57.57% <0.00%> (ø)` | |
   | [banyand/stream/metadata.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC9zdHJlYW0vbWV0YWRhdGEuZ28=) | `53.54% <0.00%> (ø)` | |
   | [banyand/tsdb/series\_seek.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC90c2RiL3Nlcmllc19zZWVrLmdv) | `0.00% <0.00%> (ø)` | |
   | [banyand/measure/measure\_query.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC9tZWFzdXJlL21lYXN1cmVfcXVlcnkuZ28=) | `0.00% <0.00%> (ø)` | |
   | [banyand/tsdb/series\_seek\_sort.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/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-YmFueWFuZC90c2RiL3Nlcmllc19zZWVrX3NvcnQuZ28=) | `0.00% <0.00%> (ø)` | |
   | ... and [11 more](https://codecov.io/gh/apache/skywalking-banyandb/pull/236/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064253459


##########
banyand/measure/measure_suite_test.go:
##########
@@ -102,7 +105,6 @@ func setUp() (*services, func()) {
 	flags = append(flags, "--etcd-listen-client-url="+listenClientURL, "--etcd-listen-peer-url="+listenPeerURL)
 	moduleDeferFunc := test.SetupModules(
 		flags,
-		repo,

Review Comment:
   https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064253100



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1059976876


##########
banyand/discovery/discovery.go:
##########
@@ -33,6 +33,7 @@ type ServiceRepo interface {
 	run.Unit
 	bus.Subscriber
 	bus.Publisher
+	GracefulStop()

Review Comment:
   Use `run.Service` in place of `GracefulStop`. The lifecycle framework will stop the repo gracefully.



##########
banyand/liaison/grpc/registry_test.go:
##########
@@ -193,12 +200,16 @@ func setupForRegistry() func() {
 		preloadStreamSvc,
 		tcp,
 	)
+	moduleDeferFunc := func() {

Review Comment:
   Could you remove this defer function? If the repo uses the lifecycle framework to stop, `deferFunc` will help stop this repo. You don't have to add a separate one.



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064144794


##########
pkg/test/setup/setup.go:
##########
@@ -91,7 +91,6 @@ func modules(flags []string) func() {
 
 	return test.SetupModules(
 		flags,
-		repo,

Review Comment:
   ok



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1063005617


##########
banyand/discovery/discovery.go:
##########
@@ -33,6 +33,7 @@ type ServiceRepo interface {
 	run.Unit
 	bus.Subscriber
 	bus.Publisher
+	GracefulStop()

Review Comment:
   Add `stopCh` to ServiceRepo as https://github.com/apache/skywalking-banyandb/blob/main/banyand/queue/local.go#L48 did. 



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1064155040


##########
banyand/measure/measure_suite_test.go:
##########
@@ -102,7 +105,6 @@ func setUp() (*services, func()) {
 	flags = append(flags, "--etcd-listen-client-url="+listenClientURL, "--etcd-listen-peer-url="+listenPeerURL)
 	moduleDeferFunc := test.SetupModules(
 		flags,
-		repo,

Review Comment:
   Do you have any advice?



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on code in PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#discussion_r1062628963


##########
banyand/discovery/discovery.go:
##########
@@ -33,6 +33,7 @@ type ServiceRepo interface {
 	run.Unit
 	bus.Subscriber
 	bus.Publisher
+	GracefulStop()

Review Comment:
   I have change ,but i don‘t know how to implement `Serve()` method of run.Service.



-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
mikechengwei commented on PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236#issuecomment-1382824659

   @hanahmily  please help me review it again, thanks for a lot .


-- 
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 #236: Fix liaison/grpc's leaked goroutines

Posted by GitBox <gi...@apache.org>.
hanahmily merged PR #236:
URL: https://github.com/apache/skywalking-banyandb/pull/236


-- 
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