New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: query flight rpc connection leaking #13956

Merged

BohuTANG merged 13 commits into databendlabs:main from dantengsky:tmp-checkrt

Dec 14, 2023

Member

dantengsky commented Dec 8, 2023 •

edited

Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

fix rpc connection leak:

use the current runtime while building FlightExchange::Receiver in flight_client
introduce WatchNotify
which provides the similar functionality of Notify, excepts that it allows notify_waiters be called before Notified future is instantiated or being polled, without losing wakeups.
It replaced the Notify used in flight_client, and ExecutorTasksQueue, PipelineExecutor and RangeJoinState
Closes #issue

This change is

github-actions bot added the pr-bugfix label

dantengsky added the ci-benchmark label

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-c3d54d5

note: this image tag is only available for internal use,
please check the internal doc for more details.

This comment was marked as outdated.

Sign in to view

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-d65dc65

note: this image tag is only available for internal use,
please check the internal doc for more details.

This comment was marked as outdated.

Sign in to view

dantengsky force-pushed the tmp-checkrt branch from e670fbe to 07877b8 Compare

December 8, 2023 07:58

dantengsky added ci-benchmark and removed ci-benchmark labels

dantengsky changed the title ~~fix: use current rt~~ fix: use current rt to create FlightExchange

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-e0446f2

note: this image tag is only available for internal use,
please check the internal doc for more details.

dantengsky changed the title ~~fix: use current rt to create FlightExchange~~ fix: use current rt to create FlightExchange::Receiver

This comment was marked as outdated.

Sign in to view

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-e0446f2

note: this image tag is only available for internal use,
please check the internal doc for more details.

This comment was marked as outdated.

Sign in to view

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-0a06647

note: this image tag is only available for internal use,
please check the internal doc for more details.

This comment was marked as outdated.

Sign in to view

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 8, 2023

Docker Image for PR

tag: pr-13956-5051d7c

note: this image tag is only available for internal use,
please check the internal doc for more details.

This comment was marked as outdated.

Sign in to view

dantengsky added 3 commits

December 10, 2023 22:37


          fix: create FlightReceiver using current rt

6a0dbe6


          fix: use notify_one in Flightreceiver::close

801f24f

instead of `notify_waiters`, so that the task spawned in
`streaming_receiver` shall not missed the notification, even if the
notification is sent before waiting


          add some comments about the notification receiving

587e4a7

dantengsky added 2 commits

December 10, 2023 22:46


          introduce WatchNotify


          lint

dantengsky force-pushed the tmp-checkrt branch from 5e768a8 to 1882159 Compare

December 10, 2023 14:47

dantengsky added 4 commits

December 10, 2023 22:48


          resolve conflict

b2ef5cb

wip

6d71141


          Revert "fix: create FlightReceiver using current rt"

ec21fc9

This reverts commit 6a0dbe6.


          cleanup

831c185

dantengsky force-pushed the tmp-checkrt branch from 4222384 to 831c185 Compare

December 10, 2023 16:34

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 10, 2023

Docker Image for PR

tag: pr-13956-324f605

note: this image tag is only available for internal use,
please check the internal doc for more details.

dantengsky changed the title ~~fix: use current rt to create FlightExchange::Receiver~~ fix: query flight rpc connection leaking


          add ut for WatchNotify

8afef56

Contributor

github-actions bot commented Dec 10, 2023

ClickBench Report

dantengsky added ci-benchmark and removed ci-benchmark labels

Contributor

github-actions bot commented Dec 10, 2023

Docker Image for PR

tag: pr-13956-324f605

note: this image tag is only available for internal use,
please check the internal doc for more details.

Contributor

github-actions bot commented Dec 10, 2023

ClickBench Report

dantengsky marked this pull request as ready for review

December 13, 2023 09:47

dantengsky requested review from zhang2014 and BohuTANG

December 13, 2023 09:47

Contributor

github-actions bot commented Dec 13, 2023

Pull request description must contain CLA like the following:

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

## Summary

Summary about this PR

- Close #issue

dantengsky requested review from SkyFan2002 and lichuang

December 13, 2023 09:48

SkyFan2002 approved these changes

View reviewed changes

zhang2014 reviewed

View reviewed changes

src/query/service/src/pipelines/executor/executor_watch_notify.rs Outdated Show resolved Hide resolved


          move WatchNotify to common/base

48b7877

lichuang approved these changes

View reviewed changes

zhang2014 approved these changes

View reviewed changes

BohuTANG merged commit e76a65e into databendlabs:main

66 of 68 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-benchmark pr-bugfix