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: implement balancer at streaming coord #34435

Merged
merged 1 commit into from
Jul 11, 2024
Merged

enhance: implement balancer at streaming coord #34435

merged 1 commit into from
Jul 11, 2024

Conversation

chyezh
Copy link
Contributor

@chyezh chyezh commented Jul 5, 2024 •
edited
Loading

issue: #33285

  • add balancer implementation
  • add channel count fair balance policy
  • add channel assignment discover grpc service

@sre-ci-robot sre-ci-robot added area/compilation size/XXL Denotes a PR that changes 1000+ lines. labels Jul 5, 2024
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chyezh
To complete the pull request process, please assign czs007 after the PR has been reviewed.
You can assign the PR to them by writing /assign @czs007 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Jul 5, 2024
@chyezh chyezh mentioned this pull request Jul 5, 2024
Open
46 tasks
@chyezh chyezh force-pushed the feat_streaming_service_coord_server branch 4 times, most recently from 11b809a to 4032087 Compare July 6, 2024 07:20
@mergify mergify bot added the ci-passed label Jul 6, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.78505% with 72 lines in your changes missing coverage. Please review.

Project coverage is 84.38%. Comparing base (80b620e) to head (68747de).
Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #34435      +/-   ##
==========================================
+ Coverage   84.36%   84.38%   +0.02%     
==========================================
  Files         849      876      +27     
  Lines      114810   115881    +1071     
==========================================
+ Hits        96857    97784     +927     
- Misses      13679    13786     +107     
- Partials     4274     4311      +37
Files Coverage Δ
internal/metastore/catalog.go 100.00% <ø> (ø)
...streamingcoord/server/balancer/channel/pchannel.go 100.00% <100.00%> (ø)
...rnal/streamingcoord/server/balancer/policy/init.go 100.00% <100.00%> (ø)
...oord/server/balancer/policy/pchannel_count_fair.go 100.00% <100.00%> (ø)
internal/streamingcoord/server/balancer/request.go 100.00% <100.00%> (ø)
...nternal/streamingcoord/server/resource/resource.go 100.00% <100.00%> (ø)
...nal/streamingcoord/server/resource/test_utility.go 100.00% <100.00%> (ø)
...er/service/discover/discover_grpc_server_helper.go 100.00% <100.00%> (ø)
...util/streamingutil/typeconverter/streaming_node.go 100.00% <100.00%> (ø)
internal/util/streamingutil/util/topic.go 100.00% <100.00%> (ø)
... and 13 more

... and 22 files with indirect coverage changes

@mergify mergify bot removed the ci-passed label Jul 7, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 7, 2024

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

@chyezh
enhance: implement balancer at streaming coord
68747de 
- add balancer implementation
- add channel count fair balance policy
- add discover grpc service

Signed-off-by: chyezh <[email protected]>
@chyezh chyezh force-pushed the feat_streaming_service_coord_server branch from 5cf73bd to 68747de Compare July 9, 2024 01:53
@mergify mergify bot added the ci-passed label Jul 9, 2024
jaime0815
jaime0815 reviewed Jul 11, 2024
View reviewed changes
internal/metastore/kv/streamingcoord/kv_catalog.go
// SavePChannels saves a pchannel
func (c *catalog) SavePChannels(ctx context.Context, infos []*streamingpb.PChannelMeta) error {
kvs := make(map[string]string, len(infos))
for _, info := range infos {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to exceed the maximum txn number of etcd, which requires executing multiple saves with multiple batches. you can refer to SaveByBatch method in datacoord catalog

internal/proto/streaming.proto
}

// PChannelMetaHistory is the history meta information of a pchannel, should only keep the data that is necessary to persistent.
message PChannelMetaHistory {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest renaming PChannelMetaHistory to PChannelAssignmentLog

internal/streamingcoord/server/balancer/balancer.go
// Balancer should be thread safe.
type Balancer interface {
// WatchBalanceResult watches the balance result.
WatchBalanceResult(ctx context.Context, cb func(version typeutil.VersionInt64Pair, relations []types.PChannelInfoAssigned) error) 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 method name to WatchChannelAssignments?

@jaime0815
Copy link
Contributor

/lgtm
Fix unresolved issues in the next PR.

@jaime0815 jaime0815 merged commit 1bc3c0b into milvus-io:master Jul 11, 2024
11 of 12 checks passed
@chyezh chyezh deleted the feat_streaming_service_coord_server branch July 11, 2024 01:58
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

@XuanYang-cn XuanYang-cn Awaiting requested review from XuanYang-cn

Assignees

@jaime0815 jaime0815

Labels
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 @sre-ci-robot @jaime0815