Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
AlexStocks merged PR #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
ywxzm03 commented on code in PR #961:
URL: https://github.com/apache/dubbo-go-pixiu/pull/961#discussion_r3377496927
##
pkg/server/cluster_manager.go:
##
@@ -942,7 +952,7 @@ func (s *ClusterStore) DeleteEndpoint(clusterName string,
endpointID string) {
if e.ID == endpointID {
runtimeCluster.RemoveEndpoint(e)
clusterConfig.Endpoints =
append(clusterConfig.Endpoints[:i], clusterConfig.Endpoints[i+1:]...)
- s.prepareClusterConfig(clusterConfig)
+ s.prepareOwnedClusterConfig(clusterConfig)
Review Comment:
引入了slices.Delete来解决删除中间元素和结尾元素带来的内存泄露隐患，但需要最小go版本1.21，本项目版本为1.25.0；如果需要兼容老版本，也可以改为手写的slices.Delete
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
sonarqubecloud[bot] commented on PR #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961#issuecomment-4655463484 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
mochengqian commented on code in PR #961:
URL: https://github.com/apache/dubbo-go-pixiu/pull/961#discussion_r3369853743
##
pkg/server/cluster_manager.go:
##
@@ -942,7 +952,7 @@ func (s *ClusterStore) DeleteEndpoint(clusterName string,
endpointID string) {
if e.ID == endpointID {
runtimeCluster.RemoveEndpoint(e)
clusterConfig.Endpoints =
append(clusterConfig.Endpoints[:i], clusterConfig.Endpoints[i+1:]...)
- s.prepareClusterConfig(clusterConfig)
+ s.prepareOwnedClusterConfig(clusterConfig)
Review Comment:
DeleteEndpoint 现在删除后直接走 prepareOwnedClusterConfig，不再像旧路径那样通过
prepareClusterConfig 全量 clone。这样 append(clusterConfig.Endpoints[:i],
clusterConfig.Endpoints[i+1:]...) 缩短 slice 后不会清理 backing array 的尾部指针。删除尾部
endpoint，尤其是连续删除尾部时，已删除 endpoint 及其 Metadata / LLMMeta 可能被 backing array
继续持有，直到 cluster 被替换或 slice 重新分配。
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
sonarqubecloud[bot] commented on PR #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961#issuecomment-4638658042 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
codecov-commenter commented on PR #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961#issuecomment-4638035092 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `60.0%` with `4 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 26.15%. Comparing base ([`d2e421d`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/d2e421d99ee5770798ad82f3f1efe3fcf496035f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6b2da67`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/6b2da67eee0e6596efd21f5122e478f3ce53fe42?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). | [Files with missing lines](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [pkg/server/cluster\_manager.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961?src=pr&el=tree&filepath=pkg%2Fserver%2Fcluster_manager.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NlcnZlci9jbHVzdGVyX21hbmFnZXIuZ28=) | 60.00% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff@@ ## develop #961 +/- ## Coverage26.14% 26.15% Files 275 275 Lines2198121987+6 + Hits 5748 5750+2 - Misses 1562315625+2 - Partials 610 612+2 ``` | [Flag](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.15% <60.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/961?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
Copilot commented on code in PR #961:
URL: https://github.com/apache/dubbo-go-pixiu/pull/961#discussion_r3367018494
##
pkg/server/cluster_manager.go:
##
@@ -356,23 +356,39 @@ func (s *ClusterStore) AddCluster(c *model.ClusterConfig)
{
stopClusters([]*cluster.Cluster{s.replaceClusterRuntime(c.Name, c)})
}
-// prepareClusterConfig rebuilds endpoint defaults and hash from current
endpoints.
+// prepareClusterConfig clones operator-supplied endpoints, then rebuilds
+// endpoint defaults and hash from current endpoints.
func (s *ClusterStore) prepareClusterConfig(c *model.ClusterConfig) {
+ if c == nil {
+ return
+ }
+ c.Endpoints = model.CloneEndpoints(c.Endpoints)
+ s.prepareOwnedClusterConfig(c)
+}
+
+// prepareOwnedClusterConfig rebuilds endpoint defaults and hash for endpoints
+// already owned by ClusterStore. Callers must not pass operator-owned endpoint
+// pointers here; use prepareClusterConfig at external input boundaries.
+func (s *ClusterStore) prepareOwnedClusterConfig(c *model.ClusterConfig) {
+ if c == nil {
+ return
+ }
s.assembleClusterEndpoints(c)
c.CreateConsistentHash()
}
// assembleClusterEndpoints assembles the cluster endpoints by formatting the
// ID, name and domains for each endpoint. If endpoint.LLMMeta is not nil, the
// assimilation of name and domain is based on the LLM provider denoted in the
-// endpoint LLMMeta. The store first deep-clones c.Endpoints, so ID/name
-// defaulting never mutates operator-supplied *model.Endpoint values.
+// endpoint LLMMeta. Callers choose the ownership boundary before invoking this
+// helper: external input paths clone endpoints in prepareClusterConfig, while
+// store-owned mutation paths call prepareOwnedClusterConfig to avoid a second
+// full endpoint clone before snapshot publication.
Review Comment:
The assembleClusterEndpoints doc comment still claims it formats endpoint
"domains" and performs name/domain "assimilation", but the function only
assigns default IDs and names. This mismatch is confusing and makes the
ownership-boundary guidance harder to follow; please update the comment to
reflect the actual behavior.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
sonarqubecloud[bot] commented on PR #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961#issuecomment-4630570291 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=961&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=961&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=961) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[PR] Feat(cluster): reduce endpoint cloning [dubbo-go-pixiu]
ywxzm03 opened a new pull request, #961: URL: https://github.com/apache/dubbo-go-pixiu/pull/961 **What this PR does**: This PR reduces duplicated endpoint deep clones in the `SetEndpoint` membership churn path, while preserving the existing ownership safety boundaries. Specifically, this PR: - Keeps the snapshot-side clone. - Separates endpoint ID/name defaulting from the endpoint clone boundary. - For endpoints already owned by `ClusterStore` in the `SetEndpoint` / `DeleteEndpoint` paths, this PR uses a store-owned prepare path to avoid another full `CloneEndpoints`. - It keeps the store-side clone at external input boundaries, so Pixiu still does not mutate endpoint objects provided by callers. - Adds a membership churn benchmark that measures repeated `SetEndpoint` metadata updates with 1000 endpoints. This PR strictly follows these constraints: - Do not change endpoint ID generation. - Do not mutate user-supplied endpoint objects after `AddCluster` or `SetEndpoint`. - Do not share snapshot-owned endpoint pointers with code that may mutate them. - Do not rewrite the snapshot mechanism. **Which issue(s) this PR fixes**: Fixes #940 **Special notes for your reviewer**: This PR chooses the second approach suggested in the issue: ```text Keep snapshot-side clone, move ID/name defaulting to a point where caller-owned objects are not mutated. ``` It does not choose the first approach: ```text Keep store-side clone, make snapshot publication reuse safe already-owned endpoint objects if possible. ``` Why the first approach was not chosen: Keeping the store-side clone and allowing snapshots to reuse store-owned endpoints can reduce one deep clone. However, `Cluster.Config` is still a publicly mutable field today. If a snapshot reuses endpoint pointers from `Config.Endpoints`, later direct mutations to `Cluster.Config.Endpoints`, `Metadata`, `LLMMeta`, or `UnHealthy` could pollute an already published runtime snapshot. To implement the first approach safely, we would likely need changes such as: - making `Cluster.Config` private and exposing it through read-only or clone-returning getters; - introducing truly immutable/frozen endpoints; - splitting the public config from the snapshot-owned endpoint source; - auditing all config mutation paths and making them copy-on-write. That scope is much larger and gets close to the “Do not rewrite the snapshot mechanism” boundary. Therefore, this PR does not choose the first approach. Instead, it keeps the snapshot-side clone and only removes the duplicated clone from store-owned mutation paths. **Verification**: ```bash go test ./pkg/server/... ./pkg/cluster ./pkg/model/... go test -run '^$' -bench '^BenchmarkClusterSetEndpointMembershipChurn$' -benchmem ./pkg/server ``` Local benchmark results before and after this change: Before: ```text goos: darwin goarch: arm64 pkg: github.com/apache/dubbo-go-pixiu/pkg/server cpu: Apple M5 BenchmarkClusterSetEndpointMembershipChurn-10 2100 478004 ns/op 1435526 B/op 9060 allocs/op PASS ok github.com/apache/dubbo-go-pixiu/pkg/server 2.198s ``` After: ```text goos: darwin goarch: arm64 pkg: github.com/apache/dubbo-go-pixiu/pkg/server cpu: Apple M5 BenchmarkClusterSetEndpointMembershipChurn-10 2830 358501 ns/op 947546 B/op 6061 allocs/op PASS ok github.com/apache/dubbo-go-pixiu/pkg/server 1.562s ``` **Does this PR introduce a user-facing change?** ```release-note NONE ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
