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

release-2.0: importccl: Preserve '\r\n' during CSV import #28225

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

dt
Copy link
Member

@dt dt commented Aug 3, 2018

Backport 1/1 commits from #28181.

/cc @cockroachdb/release


See #25344.

@dt dt requested a review from benesch August 3, 2018 01:56
@dt dt requested a review from a team as a code owner August 3, 2018 01:56
@dt dt requested a review from a team August 3, 2018 01:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Backport LGTM but please double-check with @bdarnell!

Copy link
Contributor

@neeral neeral left a comment

Choose a reason for hiding this comment

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

Note: import_stmt_test.go in master was renamed from csv_test.go in branch release-2.0 -- the unit test from #28181 hasn't made it into this PR.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/util/encoding/csv/example_test.go, line 3 at r1 (raw file):

// Copyright 2015 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

The copyright headers in all copied files need to be updated to point to the license in licenses/BSD-golang.txt (see syncutil for an example)

neeral and others added 2 commits August 6, 2018 15:03
See cockroachdb#25344.

It appears this is being caused by the behaviour of Golang's
encoding/csv library, which folds \r\n into \n when reading. This was
fixed in golang/go#21201 but then reverted golang/go#22746. It appears
based on that second issue that Go is unlikely to change that behavior.

Check in the stdlib `encoding/csv` into `pkg/util` with
golang/go#22746 reverted.

Release note:
`\r\n` characters in CSV files were silently converted into `\n`. This
causes imported data to be different. This is now fixed.
Forgot to update these when vendoring the stdlib package.
Also switch the Example_ tests to test this package instead of stdlib.

Release note: none.
@dt
Copy link
Member Author

dt commented Aug 6, 2018

nice catch! cherrypicked #28283 into this to fix the lic headers.

@neeral Yeah, I'd originally just nuked the test since I thought we didn't have the larger test runner it was in on the 2.0 branch, but I looked again just now and saw the byte reader test to which i've added it.

@dt
Copy link
Member Author

dt commented Aug 6, 2018

now just need to run the flaky 2.0 test gauntlet

@dt
Copy link
Member Author

dt commented Aug 6, 2018

bors r+

craig bot pushed a commit that referenced this pull request Aug 6, 2018
27868: backport-2.0: storage: prevent unbounded raft log growth without quorum r=nvanbenschoten a=nvanbenschoten

Backport 2/2 commits from #27774.

/cc @cockroachdb/release

---

Fixes #27772.

This change adds safeguards to prevent cases where a raft log
would grow without bound during loss of quorum scenarios. It
also adds a new test that demonstrates that the raft log does
not grow without bound in these cases.

There are two cases that need to be handled to prevent the
unbounded raft log growth observed in #27772.
1. When the leader proposes a command and cannot establish a
   quorum. In this case, we know the leader has the entry in
   its log, so there's no need to refresh it with `reasonTicks`.
   To avoid this, we no longer use `refreshTicks` as a leader.
2. When a follower proposes a command that is forwarded to the
   leader who cannot establish a quorum. In this case, the
   follower can't be sure (currently) that the leader got the
   proposal, so it needs to refresh using `reasonTicks`. However,
   the leader now detects duplicate forwarded proposals and
   avoids appending redundant entries to its log. It does so
   by maintaining a set of in-flight forwarded proposals that
   it has received during its term as leader. This set is reset
   after every leadership change.

Both of these cases are tested against in the new
TestLogGrowthWhenRefreshingPendingCommands. Without both of
the safeguards introduced in this commit, the test fails.

Release note (bug fix): Prevent loss of quorum situations from
allowing unbounded growth of a Range's Raft log.


28225: release-2.0: importccl: Preserve '\r\n' during CSV import r=dt a=dt

Backport 1/1 commits from #28181.

/cc @cockroachdb/release

---

See #25344.


Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: neeral <neeral@users.noreply.github.com>
Co-authored-by: David Taylor <tinystatemachine@gmail.com>
@craig
Copy link
Contributor

craig bot commented Aug 6, 2018

Build succeeded

@craig craig bot merged commit 0fbae2a into cockroachdb:release-2.0 Aug 6, 2018
@dt dt deleted the backport2.0-28181 branch August 6, 2018 20:01
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.

5 participants