Skip to content
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

enhance: using streaming service in insert/upsert/flush/delete/querynode #35406

Merged
merged 5 commits into from
Aug 29, 2024
Merged

enhance: using streaming service in insert/upsert/flush/delete/querynode #35406

merged 5 commits into from
Aug 29, 2024

Conversation

chyezh
Copy link
Contributor

@chyezh chyezh commented Aug 10, 2024 •
edited
Loading

issue: #33285

  • using streaming service in insert/upsert/flush/delete/querynode
  • fixup flusher bugs and refactor the flush operation
  • enable streaming service for dml and ddl
  • pass the e2e when enabling streaming service
  • pass the integration tst when enabling streaming service

@sre-ci-robot sre-ci-robot added size/XXL Denotes a PR that changes 1000+ lines. area/dependency Pull requests that update a dependency file labels Aug 10, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 10, 2024

@chyezh Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify mergify bot added needs-dco DCO is missing in this pull request. kind/enhancement Issues or changes related to enhancement labels Aug 10, 2024
@chyezh chyezh mentioned this pull request Aug 10, 2024
Open
46 tasks
@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 10, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from 056cfe0 to 45950c5 Compare August 10, 2024 06:13
@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 10, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 12, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch 3 times, most recently from 48b4535 to e0a1d1c Compare August 20, 2024 07:31
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Aug 20, 2024
@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from e0a1d1c to 70dcb67 Compare August 20, 2024 07:39
@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 20, 2024

@chyezh Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify mergify bot added needs-dco DCO is missing in this pull request. and removed dco-passed DCO check passed. labels Aug 20, 2024
@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from 2826e9f to d459642 Compare August 21, 2024 02:14
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. labels Aug 21, 2024
@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from d459642 to f84bab4 Compare August 21, 2024 02:15
chyezh
chyezh commented Aug 21, 2024
View reviewed changes
internal/proxy/task_flush.go
// See the License for the specific language governing permissions and
// limitations under the License.

package proxy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy from impl.go

@chyezh
Copy link
Contributor Author

chyezh commented Aug 28, 2024

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 28, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@chyezh
Copy link
Contributor Author

chyezh commented Aug 28, 2024

/run-cpu-e2e

@codecov Codecov
Copy link

codecov bot commented Aug 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.96120% with 386 lines in your changes missing coverage. Please review.

Project coverage is 81.41%. Comparing base (9b96841) to head (681d74e).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
internal/proxy/task_upsert_streaming.go 0.00% 99 Missing ⚠️
internal/proxy/task_insert_streaming.go 37.70% 68 Missing and 8 partials ⚠️
internal/proxy/task_flush_streaming.go 71.05% 14 Missing and 8 partials ⚠️
internal/querynodev2/delegator/delegator_data.go 44.73% 18 Missing and 3 partials ⚠️
pkg/streaming/util/message/adaptor/message.go 73.01% 11 Missing and 6 partials ⚠️
tests/integration/minicluster_v2.go 48.38% 13 Missing and 3 partials ⚠️
...ingnode/server/flusher/flusherimpl/flusher_impl.go 70.58% 11 Missing and 4 partials ⚠️
...er/wal/interceptors/segment/stats/stats_manager.go 31.57% 12 Missing and 1 partial ⚠️
internal/datacoord/services.go 63.33% 8 Missing and 3 partials ⚠️
.../interceptors/segment/manager/partition_manager.go 0.00% 11 Missing ⚠️
... and 20 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35406      +/-   ##
==========================================
+ Coverage   81.31%   81.41%   +0.09%     
==========================================
  Files        1238     1252      +14     
  Lines      148082   149159    +1077     
==========================================
+ Hits       120415   121432    +1017     
+ Misses      22888    22878      -10     
- Partials     4779     4849      +70
Files with missing lines Coverage Δ
internal/datacoord/channel_manager.go 86.52% <100.00%> (+0.06%) ⬆️
internal/datacoord/server.go 74.20% <100.00%> (+2.41%) ⬆️
internal/datanode/data_node.go 78.49% <100.00%> (+0.11%) ⬆️
internal/distributed/streaming/streaming.go 100.00% <ø> (+75.00%) ⬆️
internal/distributed/streaming/util.go 83.63% <100.00%> (+11.52%) ⬆️
internal/distributed/streaming/wal.go 66.66% <100.00%> (+25.49%) ⬆️
internal/flushcommon/pipeline/data_sync_service.go 69.43% <100.00%> (ø)
...rnal/flushcommon/pipeline/flow_graph_write_node.go 85.29% <100.00%> (+0.21%) ⬆️
internal/flushcommon/util/util.go 33.33% <ø> (ø)
internal/proxy/task.go 89.75% <ø> (-0.47%) ⬇️
... and 57 more

... and 65 files with indirect coverage changes

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 28, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@chyezh
Copy link
Contributor Author

chyezh commented Aug 28, 2024

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 28, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@jaime0815
Copy link
Contributor

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 28, 2024

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from 58d9c25 to 4c702bc Compare August 28, 2024 09:11
@mergify mergify bot added the ci-passed label Aug 28, 2024
chyezh added 5 commits August 28, 2024 19:33
@chyezh
enhance: enable streaming
d35ffc8 
- fixup flusher bugs and refactor the flush operation
- enable streaming service for dml and ddl
- pass the e2e when enabling streaming service
- pass the integration tst when enabling streaming service

Signed-off-by: chyezh <[email protected]>
Signed-off-by: bigsheeper <[email protected]>
@chyezh
fix: fix lint and ut and error handling
6fa27c4 
Signed-off-by: chyezh <[email protected]>
@chyezh
fix: revert some commit file
e9a0409 
Signed-off-by: chyezh <[email protected]>
@chyezh
fix: fix unitest and previous bug
499f1b6 
Signed-off-by: chyezh <[email protected]>
@chyezh
fix: MessageTooBig
681d74e 
Signed-off-by: chyezh <[email protected]>
@chyezh chyezh force-pushed the feat_streaming_service_insert_message_with_flusher branch from 4c702bc to 681d74e Compare August 28, 2024 11:34
@mergify mergify bot removed the ci-passed label Aug 28, 2024
jaime0815
jaime0815 reviewed Aug 28, 2024
View reviewed changes
internal/util/pipeline/stream_pipeline.go
@@ -70,6 +76,30 @@ func (p *streamPipeline) ConsumeMsgStream(position *msgpb.MsgPosition) error {
return ErrNilPosition
}

if streamingutil.IsStreamingServiceEnabled() {
startFrom := adaptor.MustGetMessageIDFromMQWrapperIDBytes("pulsar", position.GetMsgID())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transition hard code in beta streaming service componment

internal/streamingnode/server/wal/interceptors/segment/manager/partition_manager.go Show resolved Hide resolved
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: chyezh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chyezh
Copy link
Contributor Author

chyezh commented Aug 28, 2024

rerun ut

@mergify mergify bot added the ci-passed label Aug 28, 2024
@chyezh
Copy link
Contributor Author

chyezh commented Aug 29, 2024

/lgtm

@sre-ci-robot
Copy link
Contributor

@chyezh: you cannot LGTM your own PR.

In response to this:

/lgtm

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 kubernetes/test-infra repository.

@jaime0815
Copy link
Contributor

/lgtm

@sre-ci-robot sre-ci-robot merged commit 99dff06 into milvus-io:master Aug 29, 2024
15 of 16 checks passed
@chyezh chyezh deleted the feat_streaming_service_insert_message_with_flusher branch August 29, 2024 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaime0815 jaime0815 jaime0815 left review comments

@bigsheeper bigsheeper Awaiting requested review from bigsheeper

@congqixia congqixia Awaiting requested review from congqixia

Assignees

@jaime0815 jaime0815

Labels
approved area/compilation area/dependency Pull requests that update a dependency file area/test ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm sig/testing size/XXL Denotes a PR that changes 1000+ lines. test/integration integration test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chyezh @jaime0815 @sre-ci-robot