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

storageccl: remove non-ReturnSST ExportRequest #69044

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

dt
Copy link
Member

@dt dt commented Aug 17, 2021

Release justification: bug fix in new functionality.

Release note: none.

@dt dt requested review from aliher1911, a team and nihalpednekar and removed request for a team August 17, 2021 17:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt changed the title storageccl: remote non-ReturnSST ExportRequest storageccl: remove non-ReturnSST ExportRequest Aug 17, 2021
@dt dt force-pushed the no-cloud-export branch from 1c655cb to 22c5f98 Compare August 17, 2021 17:50
@dt dt closed this Aug 18, 2021
@dt dt deleted the no-cloud-export branch August 18, 2021 02:01
@dt dt restored the no-cloud-export branch August 18, 2021 03:52
@dt dt reopened this Aug 18, 2021
@dt dt force-pushed the no-cloud-export branch from 22c5f98 to a6c2c4b Compare August 18, 2021 04:02
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

What happens during upgrades if caller is unaware? It would fail until it is upgraded and then it would not retry with ReturnSST = true?

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

@dt
Copy link
Member Author

dt commented Aug 20, 2021

During upgrade we typically plan the sql procs that send export requests on the same nodes as the leaseholders so they send to themselves. Only if we somehow get a leaseholder move mid backup across versions so we expect a mismatch. In that case if they have set the old always proxy writes setting their old nodes will ask for ReturnSST and should be fine. If they haven’t, their backup might fail and they would need to enable that setting to try again (or finish rolling out the new binary)

@dt
Copy link
Member Author

dt commented Aug 20, 2021

I just realized i never backported #66540. Hm.

@dt dt force-pushed the no-cloud-export branch from a6c2c4b to 67ad54f Compare August 23, 2021 18:49
@dt
Copy link
Member Author

dt commented Aug 23, 2021

Fixing a (potential) bug while I'm here and adding an extra check that the caller didn't ask for/expect the file to be encrypted, which is a potential if an older tenant pod asked mid-upgrade. Think I'll land this and get a 66540 backport going.

@dt
Copy link
Member Author

dt commented Aug 24, 2021

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2021

Build succeeded:

@craig craig bot merged commit 7c36a9d into cockroachdb:master Aug 24, 2021
@dt dt deleted the no-cloud-export branch August 24, 2021 15:12
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.

3 participants