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

vstreamer to throttle on source endpoint #7324

Merged

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Jan 20, 2021

Description

VReplication works by hooking to a source tablet, pulling data as a replica as well as scanning tables, then to be applied on a target tablet.

On source, VReplication (via vstreamer) can cause a high load by aggressively pulling data, such that:

  • if source is a replica, it can start lagging
  • if source is a primary, it becomes sluggish

With this PR, vstreamer throttles based on check-self, introduced in #7319 , which is a throttle endpoint that tests the health of a specific tablet, the source tablet in our scenario. Right now, as described in #7319, whether replica or primary, we test for replication lag, which is an actual indicator on primary, too.

Related Issue(s)

Tracking issue: #7362

This PR extends and depends on #7319

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build
  • VTAdmin

…rapping events channel with throttledEvents, which only pulls data from events if not throttled

Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

@rohit-nayak-ps please see my recent changes. On vstreamer.go, I only throttle on the events channel, so that I'm not blocking venets and ctx.Done() and timer.C.

Otherwise, added throttling in rowstreamer and resultstreamer.

@shlomi-noach
Copy link
Contributor Author

Next up, I'l create an endtoend test

Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

I've moved rowstreamer and resultstreamer throttling inside their main loops, where throttling is more granular. In my understanding, where we initially placed throttling (at beginning of Stream() function) was a one time check at the point of initially setting up the streaming.

Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach shlomi-noach changed the title WIP: vstreamer to throttle on source endpoint vstreamer to throttle on source endpoint Jan 24, 2021
@shlomi-noach shlomi-noach marked this pull request as ready for review January 24, 2021 11:08
@shlomi-noach
Copy link
Contributor Author

Ready for review! Tests are now good.

Reminder that this PR extends #7319, so it's best to first merge #7319

@rohit-nayak-ps
Copy link
Contributor

rohit-nayak-ps commented Jan 25, 2021

The new workflow yaml file for the e2e test should be created by updating the test/ci_workflow_gen.go codegen script and running it. Otherwise any updates to the cluster template file will not reflect in this yaml.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

looks great!
test script / e2e workflow yaml needs to be updated as per comment.

@shlomi-noach
Copy link
Contributor Author

The new workflow yaml file for the e2e test should be created by updating the test/ci_workflow_gen.go codegen script

Oh, I completely missed this!

@shlomi-noach
Copy link
Contributor Author

because this PR introduced a named endtoend shard (vreplication_basic), I took it to now normalize all shards to be strings, not numbers. So, for example, shard 22 is now shard "22".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants