Copilot commented on code in PR #3305: URL: https://github.com/apache/dubbo-go/pull/3305#discussion_r3367024336
########## cluster/router/tag/cache.go: ########## @@ -0,0 +1,212 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package tag + +import ( + "strings" +) + +import ( + "github.com/RoaringBitmap/roaring" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/cluster/router" + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" +) + +func (p *PriorityRouter) Name() string { + return "tag" +} + +func (p *PriorityRouter) ShouldPool() bool { + return true +} + +// Pool builds bitmap indices for tag, address, and port keys. +// Key space uses "\x00" as separator. +// Other Poolable routers should use different prefixes to avoid conflicts. +func (p *PriorityRouter) Pool(invokers []base.Invoker) (router.AddrPool, router.AddrMetadata) { + pool := make(router.AddrPool) + for i, invoker := range invokers { + url := invoker.GetURL() + upsertBM(pool, constant.PoolKeyAll, i) + tag := url.GetParam(constant.Tagkey, "") + upsertBM(pool, constant.PoolKeyTagPrefix+tag, i) + addr := url.Location + if addr != "" { + upsertBM(pool, constant.PoolKeyAddrPrefix+addr, i) + if idx := strings.LastIndex(addr, ":"); idx > 0 { + upsertBM(pool, constant.PoolKeyPortPrefix+addr[idx+1:], i) + } + } + } + return pool, nil +} + +func (p *PriorityRouter) SetCache(cache router.Cache) { + if cache != nil { + p.cache.Store(cache) + } +} + +func (p *PriorityRouter) routeWithPool(invokers []base.Invoker, pool router.AddrPool, url *common.URL, invocation base.Invocation) []base.Invoker { + if len(invokers) == 0 { + return invokers + } + tag := invocation.GetAttachmentWithDefaultValue(constant.Tagkey, url.GetParam(constant.Tagkey, "")) + + application := invokers[0].GetURL().GetParam(constant.ApplicationKey, "") + key := strings.Join([]string{application, constant.TagRouterRuleSuffix}, "") + value, ok := p.routerConfigs.Load(key) + if !ok { + return collectInvokers(invokers, p.staticTagMatchBM(pool, tag, url, invocation)) + } + routerCfg := value.(global.RouterConfig) + enabled := routerCfg.Enabled == nil || *routerCfg.Enabled + valid := (routerCfg.Valid != nil && *routerCfg.Valid) || (routerCfg.Valid == nil && len(routerCfg.Tags) > 0) + if !enabled || !valid { + return collectInvokers(invokers, p.staticTagMatchBM(pool, tag, url, invocation)) + } + if tag == "" { + return collectInvokers(invokers, p.emptyTagMatchBM(pool, routerCfg)) + } + bm := p.requestTagMatchBM(pool, url, invocation, routerCfg, tag) + if bm == nil { + return requestTag(invokers, url, invocation, routerCfg, tag) + } + return collectInvokers(invokers, bm) +} + +func (p *PriorityRouter) staticTagMatchBM(pool router.AddrPool, tag string, url *common.URL, invocation base.Invocation) *roaring.Bitmap { + if tag != "" { + if bm := pool[constant.PoolKeyTagPrefix+tag]; bm != nil && !bm.IsEmpty() { + return bm + } + if requestIsForce(url, invocation) { + return nil + } + } + return pool[constant.PoolKeyTagPrefix] +} + +func (p *PriorityRouter) requestTagMatchBM(pool router.AddrPool, url *common.URL, invocation base.Invocation, + cfg global.RouterConfig, tag string) *roaring.Bitmap { + var ( + addresses []string + match []*common.ParamMatch + ) + for _, tagCfg := range cfg.Tags { + if tagCfg.Name == tag { + addresses = tagCfg.Addresses + match = tagCfg.Match + break + } + } + + if len(match) != 0 { + return nil // not bitmap-cached; fall back to requestTag + } Review Comment: requestTagMatchBM currently disables the bitmap fast path for *any* tag rule that uses ParamMatch (len(match)!=0), including common keys like version/group. This contradicts the PR description/issue statement that version/group can be indexed while only truly-unindexed keys fall back. Either extend the pool/indexing to cover version/group ParamMatch, or update the PR description to reflect that any ParamMatch currently forces the slower fallback path. ########## cluster/router/chain/chain.go: ########## @@ -93,12 +108,26 @@ func (c *RouterChain) AddRouters(routers []router.PriorityRouter) { func (c *RouterChain) SetInvokers(invokers []base.Invoker) { c.mutex.Lock() defer c.mutex.Unlock() + c.generation++ c.invokers = invokers + c.rebuildCache(invokers) for _, v := range c.routers { v.Notify(c.invokers) } } +func (c *RouterChain) rebuildCache(invokers []base.Invoker) { + if c.cache == nil { + c.cache = newRouterCache() + for _, r := range c.routers { + if accessor, ok := r.(router.CacheAccessor); ok { + accessor.SetCache(c.cache) + } + } + } + c.cache.rebuild(c.generation, invokers, c.routers) Review Comment: rebuildCache only calls CacheAccessor.SetCache when c.cache is nil. If RouterChain.AddRouters replaces c.routers after the cache has been initialized, any newly added Poolable+CacheAccessor router will never receive the cache reference and will be unable to use the snapshot cache. Consider always (re)setting the cache on routers, even when c.cache already exists. ########## cluster/router/router.go: ########## @@ -95,9 +95,20 @@ type Cache interface { // GetInvokers returns the snapshot of received invokers. GetInvokers() []base.Invoker - // FindAddrPool returns address pool associated with the given Poolable instance. - FindAddrPool(Poolable) AddrPool + // FindAddrPool returns the address pool, the invoker snapshot, and the generation of that + // snapshot in a single locked read. The generation lets callers verify the pool/invokers + // belong to the same generation the chain snapshotted for the current route, so the bitmap + // indices stay aligned with the invoker slice and the route is not served from a snapshot + // produced by a concurrent SetInvokers. + FindAddrPool(Poolable) (AddrPool, []base.Invoker, uint64) Review Comment: Changing the exported router.Cache interface method signature (FindAddrPool now returns (AddrPool, []Invoker, uint64)) is a breaking change for any external code that implements router.Cache. If backward compatibility is important for v3 consumers, consider keeping the original FindAddrPool(Poolable) AddrPool and adding a new method (e.g., FindAddrPoolSnapshot/FindAddrPoolWithGeneration) for the extended return values, or provide an adapter type so downstream implementations don’t break at compile time. -- 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]
