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

changefeedccl: Break gossip dependency #101096

Merged
merged 1 commit into from
Apr 18, 2023
Merged

Conversation

miretskiy
Copy link
Contributor

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

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 cockroachdb#47971

Release note: None
@miretskiy miretskiy requested a review from a team as a code owner April 10, 2023 15:29
@blathers-crl
Copy link

blathers-crl bot commented Apr 10, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @shralex)


pkg/ccl/changefeedccl/kvfeed/scanner.go line 254 at r1 (raw file):

// getRangesToProcess returns the list of ranges covering input list of spans.
// Returns the number of nodes that are leaseholders for those spans.

nit: This is not necessarily the number of leaseholders, right? it.Desc().InternalReplicas returns all the nodes hosting replicas.

@miretskiy
Copy link
Contributor Author

it: This is not necessarily the number of leaseholders, right? it.Desc().InternalReplicas returns all the nodes hosting replicas.

Do you feel strongly on this (and force rerunning CI)?

Copy link
Contributor

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

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

I do not feel strongly. Feel free to merge. :lgtm_strong:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @shralex)

@miretskiy
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 18, 2023

Build succeeded:

@craig craig bot merged commit 2c09855 into cockroachdb:master Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvfeed: break Gossip dependency
3 participants