Skip to content

messaging: support vt_message_cols to limit cols#9670

Merged
derekperkins merged 1 commit intovitessio:mainfrom
derekperkins:vt_streamed_cols
May 30, 2022
Merged

messaging: support vt_message_cols to limit cols#9670
derekperkins merged 1 commit intovitessio:mainfrom
derekperkins:vt_streamed_cols

Conversation

@derekperkins
Copy link
Member

@derekperkins derekperkins commented Feb 11, 2022

Description

this enables users to set an allow-list of columns that will be loaded into memory and streamed to subscribers when they call stream * from tbl

Related Issue(s)

#9666

Checklist

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

@derekperkins derekperkins added Component: Query Serving Component: TabletManager release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Feb 11, 2022
@derekperkins derekperkins force-pushed the vt_streamed_cols branch 2 times, most recently from 4488a43 to abfaeea Compare February 11, 2022 05:11
@derekperkins derekperkins force-pushed the vt_streamed_cols branch 3 times, most recently from 0c8d537 to b6de19f Compare March 15, 2022 05:20
@derekperkins derekperkins marked this pull request as ready for review March 15, 2022 05:20
@derekperkins derekperkins requested review from sougou and removed request for ajm188, harshit-gangal, shlomi-noach and systay March 15, 2022 05:20
@derekperkins derekperkins force-pushed the vt_streamed_cols branch 2 times, most recently from 233ce20 to b4d93cd Compare March 15, 2022 06:59
@mattlord mattlord assigned mattlord and unassigned mattlord Apr 21, 2022
@mattlord mattlord removed the request for review from GuptaManan100 April 21, 2022 16:54
@mattlord mattlord requested a review from rohit-nayak-ps April 21, 2022 16:54
@mattlord
Copy link
Member

@derekperkins I apologize for the delay, I will be reviewing this today.

@derekperkins
Copy link
Member Author

Great, thanks. I'll clean it up and get it passing tests again. I've been using this as our custom build branch trying to solve #9936. We've had this running in prod for 6 weeks or so and it has been working great.

Copy link
Member

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for working on this! This will make it feasible to have much larger message cache sizes.

I had some questions/comments/nits mostly related to naming but you can make the final call on the naming aspects. Naming things is hard, and as long as we update the docs and explain the field well that's what matters most. Speaking of that, I assume you'll have a docs PR ready to review as well? Let me know. That has gotten much easier now that we have versioned docs and I can offer help if needed.

Copy link
Member

@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.

aside from minor nits, functionality lgtm

@derekperkins
Copy link
Member Author

Review comments are all addressed now in the last commit in the last 2 commits. This is rebased on top of #10132. Once that is fixed and merged, I'll rebase again to get the tests passing

@mattlord
Copy link
Member

Review comments are all addressed now in the last commit in the last 2 commits. This is rebased on top of #10132. Once that is fixed and merged, I'll rebase again to get the tests passing

@derekperkins Should be good now, test wise. I am still working on the locking related stuff in the other PR but the tests should be done.

@derekperkins derekperkins changed the title messaging: support vt_stream_cols to limit cols messaging: support vt_message_cols to limit cols Apr 25, 2022
@derekperkins derekperkins force-pushed the vt_streamed_cols branch 6 times, most recently from 86a2572 to 584bd4d Compare April 28, 2022 20:04
@derekperkins
Copy link
Member Author

@mattlord I think this should be good to go. I ran into some rebasing issues when switching from being rebased on #10132 to main, not realizing you had squashed. In the end, it was easiest for me to just squash mine too, vs leaving the separate review commit. I addressed all the review items, via name changes and comments. May be worth a quick once over again, but no major functionality changes.

Copy link
Member

@mattlord mattlord 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! So much easier to read for me. Thank you for taking the time to make those changes, add comments, etc. It's not only easier to read, but more resilient and easier to use as well. ❤️

I had one minor question about the table structure used — hoping we can base it on what we now have in the docs — but that's it.

Thanks again!

@mattlord
Copy link
Member

Oh, one last thing... will you be opening a website PR as well so that we add details on this new configuration parameter to the v14 docs here: https://github.com/vitessio/website/blob/prod/content/en/docs/14.0/reference/features/messaging.md and here: https://github.com/vitessio/website/blob/prod/content/zh/docs/14.0/reference/messaging.md ?

I can help with that (creation or review), just let me know.

@mattlord
Copy link
Member

Friendly ping on this 🙂

@mattlord
Copy link
Member

@derekperkins ping on this. We're quickly running out of time to get this into 14.0 (RC coming soon).

@derekperkins
Copy link
Member Author

Working on the docs PR now

this enables users to set an allow-list of columns that will be loaded into memory and streamed to subscribers when they call `stream * from tbl`

Signed-off-by: Derek Perkins <derek@nozzle.io>
@derekperkins derekperkins merged commit c4878e9 into vitessio:main May 30, 2022
@derekperkins derekperkins deleted the vt_streamed_cols branch May 30, 2022 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Query Serving Component: TabletManager release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants