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

Added note to 19.1 docs about turning off range merging for manual splits #5642

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Oct 17, 2019

Fixes #4960.

Note that this affects only 19.1 docs, since cockroachdb/cockroach#38004 removes this limitation in 19.2.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@bdarnell
Copy link
Contributor

We should probably have a companion upgrade checklist: when upgrading to 19.2, check your cluster settings:

  • If you disabled the range merge queue in order to use manual splits, consider turning it back on since manual splits no longer conflict with the range queue
  • etc.

…splits, and turning them back on when upgrading

- Removed sections about when to disable/enable range merges sections in 19.2
@ericharmeling
Copy link
Contributor Author

@bdarnell Thanks for the suggestion Ben! I updated the PR:

  • Added a note about turning merging back on when upgrading (in SPLIT AT and in Range Merges docs)
  • Removed the sections about "When to turn merging on/off", as they don't really add any information or apply in 19.2

I'm happy to add/remove any more information.

@bdarnell
Copy link
Contributor

I was thinking the note about removing the setting in upgrading would go in https://www.cockroachlabs.com/docs/dev/upgrade-cockroach-version.html

@ericharmeling
Copy link
Contributor Author

I was thinking the note about removing the setting in upgrading would go in https://www.cockroachlabs.com/docs/dev/upgrade-cockroach-version.html

Looking at that page, I'm not sure exactly where putting a checklist like that should go in the current structure of the page. It might be best to create a new section at the bottom of the page (something like "Post-upgrade Checklist"), with some suggestions about changing settings for performance optimization and new features in 19.2. That probably should be a separate issue and PR. That separate issue might also inform the issue that @jseldess opened about finalizing an upgrade: #5646

@jseldess Thoughts?

@jseldess
Copy link
Contributor

If this action needs to be taken after finalizing the upgrade, I agree that we could either add it to https://www.cockroachlabs.com/docs/dev/upgrade-cockroach-version.html#step-5-finish-the-upgrade, or create a subsequent step.

@ericharmeling
Copy link
Contributor Author

@bdarnell

We should probably have a companion upgrade checklist: when upgrading to 19.2, check your cluster settings:

  • If you disabled the range merge queue in order to use manual splits, consider turning it back on since manual splits no longer conflict with the range queue
  • etc.

A couple follow-up questions:

  • Is there a reason why reseting kv.range_merge.queue_enabled is not automated as a part of the upgrade finalization process?
  • What other cluster settings (the "etc." you mentioned above) need checking/updating post-upgrade?
  • Are there any other post-upgrade steps that you know of that we are not documenting on Upgrade to CockroachDB v19.2?

@bdarnell
Copy link
Contributor

Is there a reason why reseting kv.range_merge.queue_enabled is not automated as a part of the upgrade finalization process?

Because they might have turned it off for other reasons. We should be very cautious about automatically undoing user actions.

What other cluster settings (the "etc." you mentioned above) need checking/updating post-upgrade?

Good question. I don't know if there are any, but the "known limitation" list might be a good place to look. Any known limitation which A) had a workaround and B) is resolved in 19.2 would be worth listing here to let users know that the workaround can be removed.

@ericharmeling
Copy link
Contributor Author

Thanks @bdarnell. I opened a new issue for the update to the Upgrade to CockroachDB v19.2 page, as I think it's out-of-scope for this PR (see #5672). I will open a separate PR to resolve that separate issue this week.

If you (or @andreimatei) are fine with the other changes in this PR, I can pass it off to @rmloveland to review for merging.

@ericharmeling ericharmeling requested review from rmloveland and removed request for rmloveland October 23, 2019 16:54
@jseldess
Copy link
Contributor

@bdarnell, you ok with this PR if we follow-up with a post-upgrade checklist?

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Yes, it's fine to leave a post-upgrade checklist as a separate item.

Copy link
Contributor

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

LGTM

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.

SPLIT AT Doc Update
5 participants