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

kvfeed: break Gossip dependency #47971

Closed
tbg opened this issue Apr 23, 2020 · 6 comments · Fixed by #101096
Closed

kvfeed: break Gossip dependency #47971

tbg opened this issue Apr 23, 2020 · 6 comments · Fixed by #101096
Assignees
@miretskiy
Labels
A-multitenancy Related to multi-tenancy T-cdc X-anchored-telemetry The issue number is anchored by telemetry references.

Comments

@tbg
Copy link
Member

tbg commented Apr 23, 2020 •
edited by knz
Loading

Background

I believe the only actual dependency is here:

cockroach/pkg/ccl/changefeedccl/kvfeed/scanner.go

Line 245 in f176a39

_ = g.IterateInfos(gossip.KeyNodeIDPrefix, func(_ string, _ gossip.Info) error {

It should be easy to avoid it by falling back to 1. We don't anticipate slurping lots of data, though we do expect to use range feeds to replace Gossip #47150, which I think (can't quite tell from first glance) will exercise this code (hence A-multitenancy-blocker).

Jira issue: CRDB-4369

Epic: CRDB-26091
@tbg tbg added A-multitenancy Related to multi-tenancy A-multitenancy-blocker labels Apr 23, 2020
@ajwerner
Copy link
Contributor

which I think (can't quite tell from first glance) will exercise this code (hence A-multitenancy-blocker).

This is higher level than any usage of range feeds that the system will depend on. This is squarely in the changefeeds realm. Falling back to 1 will be the move. I can tackle this such that a nil gossip doesn't cause problems.

@tbg
Copy link
Member Author

tbg commented Apr 23, 2020

👍 no rush, you may want to wait for #47972 until you make your move. Removing blocker label though, since it doesn't seem like a blocker at all.

@ajwerner ajwerner mentioned this issue Apr 29, 2020
Merged
@knz knz added the X-anchored-telemetry The issue number is anchored by telemetry references. label Sep 11, 2020
@knz knz mentioned this issue Sep 11, 2020
Merged
craig bot pushed a commit that referenced this issue Sep 14, 2020
@knz
Merge #54256
40f03d9 
54256: sql: make multi-tenancy errors report to telemetry r=asubiotto a=knz

Fixes #48164. 

Issues referenced, I also added the X-anchored-telemetry label to them on github (we keep those issues open on github until their reference in the code is removed):

#54254 
#54255 
#54250 
#54251 
#54252 
#49854 
#48274
#47150
#47971
#47970
#47900 
#47925 


Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@ajwerner ajwerner removed their assignment Mar 17, 2022
@blathers-crl
Copy link

blathers-crl bot commented Mar 17, 2022

cc @cockroachdb/cdc

@blathers-crl blathers-crl bot added the T-cdc label Mar 17, 2022
@ajwerner
Copy link
Contributor

@miretskiy I'm getting this out of my queue. It's about tuning the scan concurrency. Much more sophisticated mechanisms have been discussed of late. This one here we use today is silly. Maybe we've removed this already.

@miretskiy miretskiy self-assigned this Mar 23, 2022
@knz knz mentioned this issue Mar 28, 2023
Open
@knz knz removed the T-cdc label Apr 7, 2023
@exalate-issue-sync exalate-issue-sync bot added the T-kv KV Team label Apr 7, 2023
@exalate-issue-sync exalate-issue-sync bot added T-cdc and removed T-kv KV Team labels Apr 7, 2023
@blathers-crl
Copy link

blathers-crl bot commented Apr 7, 2023

cc @cockroachdb/cdc

@blathers-crl blathers-crl bot added the A-cdc Change Data Capture label Apr 7, 2023
@exalate-issue-sync exalate-issue-sync bot removed the A-cdc Change Data Capture label Apr 7, 2023
@shralex
Copy link
Contributor

shralex commented Apr 7, 2023

discussed with knz offline, the assignment changes were made by mistake, this is still a CDC issue

@miretskiy miretskiy mentioned this issue Apr 10, 2023
Merged
craig bot pushed a commit that referenced this issue Apr 18, 2023
Merge #101096
2c09855 
101096: changefeedccl: Break gossip dependency r=miretskiy a=miretskiy

Break the dependency on Gossip library which was
used to determine the number of nodes in the cluster in order to limit scanner concurrency.
Instead, rely on the range descriptors in order
to determine how many nodes host the ranges that
need to be scanned.

Fixes #47971

Release note: None

Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
@craig craig bot closed this as completed in ad7d868 Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@miretskiy miretskiy

Labels
A-multitenancy Related to multi-tenancy T-cdc X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

changefeedccl: Break gossip dependency miretskiy/cockroach
5 participants
@knz @ajwerner @tbg @shralex @miretskiy