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: streaming node client implementation #34653

Merged
merged 2 commits into from
Jul 19, 2024
Merged

enhance: streaming node client implementation #34653

merged 2 commits into from
Jul 19, 2024

Conversation

chyezh
Copy link
Contributor

@chyezh chyezh commented Jul 12, 2024

issue: #33285

  • add streaming node grpc client wrapper
  • add unittest for streaming node grpc client side
  • fix binary unsafe bug for message

@sre-ci-robot sre-ci-robot added size/XXL Denotes a PR that changes 1000+ lines. area/compilation labels Jul 12, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Jul 12, 2024
@chyezh chyezh mentioned this pull request Jul 12, 2024
Open
46 tasks
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 12, 2024

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

@chyezh chyezh force-pushed the feat_streaming_service_streaming_node_client branch 2 times, most recently from 3ffd30f to aa50e62 Compare July 16, 2024 14:05
@mergify mergify bot added the ci-passed label Jul 16, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.51883% with 111 lines in your changes missing coverage. Please review.

Project coverage is 84.41%. Comparing base (c61592d) to head (6a8c6c3).
Report is 4 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #34653      +/-   ##
==========================================
- Coverage   84.44%   84.41%   -0.03%     
==========================================
  Files         892      904      +12     
  Lines      117014   117756     +742     
==========================================
+ Hits        98807    99408     +601     
- Misses      13836    13933      +97     
- Partials     4371     4415      +44
Files Coverage Δ
...mingnode/client/handler/assignment/watcher_impl.go 100.00% <100.00%> (ø)
...ode/client/handler/producer/produce_grpc_client.go 100.00% <100.00%> (ø)
...er/resource/idalloc/test_mock_root_coord_client.go 83.33% <100.00%> (-2.39%) ⬇️
...al/util/streamingutil/service/balancer/balancer.go 76.53% <ø> (+19.38%) ⬆️
...nal/util/streamingutil/service/resolver/builder.go 92.30% <ø> (+15.38%) ⬆️
...gutil/service/resolver/resolver_with_discoverer.go 100.00% <100.00%> (+3.15%) ⬆️
pkg/util/paramtable/component_param.go 98.53% <100.00%> (+<0.01%) ⬆️
pkg/util/typeutil/shared_reference.go 97.36% <97.36%> (ø)
...nal/streamingnode/client/handler/handler_client.go 96.77% <96.77%> (ø)
...nal/streamingnode/client/manager/manager_client.go 96.42% <96.42%> (ø)
... and 7 more

... and 30 files with indirect coverage changes

@chyezh
Copy link
Contributor Author

chyezh commented Jul 17, 2024

rerun ut

1 similar comment
@chyezh
Copy link
Contributor Author

chyezh commented Jul 17, 2024

rerun ut

@chyezh chyezh force-pushed the feat_streaming_service_streaming_node_client branch from aa50e62 to 3794d3a Compare July 17, 2024 06:53
@mergify mergify bot removed the ci-passed label Jul 17, 2024
@chyezh
Copy link
Contributor Author

chyezh commented Jul 17, 2024

rerun ut

3 similar comments
@chyezh
Copy link
Contributor Author

chyezh commented Jul 17, 2024

rerun ut

@chyezh
Copy link
Contributor Author

chyezh commented Jul 18, 2024

rerun ut

@chyezh
Copy link
Contributor Author

chyezh commented Jul 18, 2024

rerun ut

@chyezh chyezh force-pushed the feat_streaming_service_streaming_node_client branch from 3794d3a to 48af2aa Compare July 18, 2024 07:28
jaime0815
jaime0815 reviewed Jul 18, 2024
View reviewed changes
internal/streamingnode/client/handler/assignment/watcher_impl.go
}

// Watch watches the channel assignment.
func (w *watcherImpl) Watch(ctx context.Context, channel string, previous *types.PChannelInfoAssigned) error {
Copy link
Contributor

@jaime0815 jaime0815 Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggest adding start and end watch logs along with time costs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

internal/streamingnode/client/handler/producer/producer_impl.go Outdated
})
return err
}
p.logger.Debug("send produce message to server", zap.Int64("requestID", requestID))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the log to prevent too many logs are generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

internal/streamingnode/client/handler/handler_client_impl.go Outdated

// createHandlerUntilStreamingNodeReady creates a handler until streaming node ready.
// If streaming node is not ready, it will block until new assignment term is coming or context timeout.
func (hc *handlerClientImpl) createHandlerUntilStreamingNodeReady(ctx context.Context, pchannel string, create func(ctx context.Context, assign *types.PChannelInfoAssigned) (any, error)) (any, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename createHandlerUntilStreamingNodeReady to createHandlerAfterStreamingNodeReady

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix it soon

@jaime0815
Copy link
Contributor

todo: refine connection granularity of consumer stream to pchannel level

@chyezh
Copy link
Contributor Author

chyezh commented Jul 18, 2024

rerun ut

@chyezh chyezh force-pushed the feat_streaming_service_streaming_node_client branch from 48af2aa to 34c9d14 Compare July 18, 2024 12:40
@chyezh
Copy link
Contributor Author

chyezh commented Jul 19, 2024

rerun ut

1 similar comment
@chyezh
Copy link
Contributor Author

chyezh commented Jul 19, 2024

rerun ut

chyezh added 2 commits July 19, 2024 11:25
@chyezh
enhance: streaming node client implementation
8b4a45a 
- add streaming node grpc client wrapper
- add unittest for streaming node grpc client side
- fix binary unsafe bug for message

Signed-off-by: chyezh <[email protected]>
@chyezh
fix: fix lint
6a8c6c3 
Signed-off-by: chyezh <[email protected]>
@chyezh chyezh force-pushed the feat_streaming_service_streaming_node_client branch from 34c9d14 to 6a8c6c3 Compare July 19, 2024 03:25
@chyezh
Copy link
Contributor Author

chyezh commented Jul 19, 2024

rerun ut

@mergify mergify bot added the ci-passed label Jul 19, 2024
@jaime0815
Copy link
Contributor

/lgtm

@jaime0815 jaime0815 added approved and removed lgtm labels Jul 19, 2024
@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

@jaime0815 jaime0815 added the lgtm label Jul 19, 2024
@sre-ci-robot sre-ci-robot merged commit 86eff6e into milvus-io:master Jul 19, 2024
11 of 12 checks passed
@chyezh chyezh deleted the feat_streaming_service_streaming_node_client branch July 19, 2024 09:43
@chyezh chyezh mentioned this pull request Jul 19, 2024
Closed
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

@godchen0212 godchen0212 Awaiting requested review from godchen0212

Assignees

@jaime0815 jaime0815

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

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