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.

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

Ftr: bitmap in router chain #708

Merged
merged 30 commits into from
Aug 28, 2020
Merged

Ftr: bitmap in router chain #708

merged 30 commits into from
Aug 28, 2020

Conversation

beiwei30
Copy link
Member

Introduce roaring bitmap in router chain. By doing this, we can avoid iterating every invoker in Route as much as possible so that we could reduce consuming both CPU and memory. At the same time, by passing the pre-built address cache along the router chain, we can keep router's implementation lock-free at the same time.

cluster/router/chain/chain.go Show resolved Hide resolved
cluster/router/router.go Show resolved Hide resolved
cluster/router/healthcheck/health_check_route.go Outdated Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/router.go Show resolved Hide resolved
cluster/router/utils/bitmap_util.go Outdated Show resolved Hide resolved
@beiwei30
Copy link
Member Author

beiwei30 commented Aug 12, 2020

Thanks Alex for the review comments. I will take a look tomorrow.

@zouyx zouyx added the pending PR Don't merge label Aug 13, 2020
cluster/router/chain/chain.go Show resolved Hide resolved
cluster/router/chain/chain.go Show resolved Hide resolved
cluster/router/chain/chain.go Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/chain/invoker_cache.go Outdated Show resolved Hide resolved
cluster/router/router.go Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
// buildCache builds address cache with the new invokers for all poolable routers.
func (c *RouterChain) buildCache() {
invokers := c.copyInvokers()
if invokers == nil || len(c.invokers) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

pls delete the condition len(c.invokers) == 0 because u have check it in c.copyInvokers().

Copy link
Member Author

Choose a reason for hiding this comment

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

this is still necessary, since the copied invokers will be null if the original invoker to be copied is null.

cluster/router/healthcheck/health_check_route.go Outdated Show resolved Hide resolved
cluster/router/chain/chain.go Show resolved Hide resolved
@zouyx zouyx removed the pending PR Don't merge label Aug 17, 2020
Copy link
Member

@zouyx zouyx left a comment

Choose a reason for hiding this comment

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

Fix the conflicts pls, thanks.

@zouyx zouyx added this to the 1.5.2 milestone Aug 17, 2020
cluster/router/chain/chain.go Outdated Show resolved Hide resolved
cluster/router/tag/tag_router.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #708 into develop will decrease coverage by 0.35%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #708      +/-   ##
===========================================
- Coverage    64.08%   63.73%   -0.36%     
===========================================
  Files          239      239              
  Lines        12753    12719      -34     
===========================================
- Hits          8173     8106      -67     
- Misses        3796     3834      +38     
+ Partials       784      779       -5     
Impacted Files Coverage Δ
cluster/router/condition/listenable_router.go 54.83% <0.00%> (ø)
cluster/router/tag/file.go 76.92% <0.00%> (ø)
common/url.go 62.89% <0.00%> (-3.06%) ⬇️
cluster/router/chain/chain.go 59.12% <49.50%> (-26.60%) ⬇️
cluster/router/chain/invoker_cache.go 50.00% <50.00%> (ø)
cluster/router/healthcheck/health_check_route.go 66.66% <76.47%> (-1.34%) ⬇️
cluster/directory/static_directory.go 65.00% <80.00%> (+0.13%) ⬆️
cluster/router/tag/tag_router.go 76.08% <84.61%> (+1.61%) ⬆️
cluster/router/condition/router.go 80.00% <88.88%> (+0.21%) ⬆️
registry/directory/directory.go 81.15% <90.47%> (+1.03%) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e361067...f606124. Read the comment docs.

@zouyx zouyx added the enhancement New feature or request label Aug 20, 2020
@zouyx zouyx changed the title bitmap in router chain Ftr: bitmap in router chain Aug 20, 2020

c.count++
now := time.Now()
if c.count >= countThreshold && now.Sub(c.last) >= timeThreshold {
Copy link
Contributor

@cvictory cvictory Aug 24, 2020

Choose a reason for hiding this comment

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

If the data from registry is changed once, it will not notify to c.notify.

And it will build cache when it is timeout or receive notify channel. The worst situation is that it cannot build cache after 5 seconds.

//cluster/router/chain/chain.go:127
	for {
		ticker := time.NewTicker(timeInterval)
		select {
		case <-ticker.C:
			c.buildCache()
		case <-c.notify:
			c.buildCache()
		}
	}

It can't be build cache as soon as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is expected.

@AlexStocks AlexStocks merged commit e6205de into apache:develop Aug 28, 2020
@zouyx zouyx mentioned this pull request Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants