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

Set datakey for first table in stream writer #1054

Merged
merged 3 commits into from
Sep 30, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Sep 25, 2019

If encryption is enabled on a DB and stream writer is used then the
first file SST would not be encrypted, only the SSTs after the first one
would be encrypted. This PR fixes it.


This change is Reviewable

If encryption is enabled on a DB and stream writer is used then the
first file SST would not be encrypted, only the SSTs after the first one
would be encrypted. This PR fixes it.
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@jarifibrahim you can click here to see the review status or cancel the code review job.

@coveralls
Copy link

coveralls commented Sep 25, 2019

Coverage Status

Coverage increased (+0.6%) to 78.471% when pulling 9bf1c0b on ibrahim/streamwriter-encryption into a425b0e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 77.691% when pulling 5608256 on ibrahim/streamwriter-encryption into a425b0e on master.

Copy link
Contributor Author

@jarifibrahim jarifibrahim 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: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)


stream_writer.go, line 128 at r2 (raw file):

	}

	for streamID, req := range streamReqs {

This was changed to make golint happy. See #1054 (comment)


stream_writer.go, line 216 at r2 (raw file):

}

func (sw *StreamWriter) newWriter(streamID uint32) (*sortedWriter, error) {

This was changed to make golint happy. See #1054 (comment)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

No additional feedback at this time.


Reviewed with ❤️ by PullRequest

Copy link
Contributor

@ashish-goswami ashish-goswami left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @jarifibrahim, and @manishrjain)


stream_writer.go, line 134 at r2 (raw file):

			writer, err = sw.newWriter(streamID)
			if err != nil {
				return errors.Wrapf(err, "failed to create writer with ID %d", streamID)

I think error can be wrapped only once.

Copy link
Contributor Author

@jarifibrahim jarifibrahim 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: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)


stream_writer.go, line 134 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

I think error can be wrapped only once.

Done. See newWriter method. It no longer wraps the error.

@jarifibrahim
Copy link
Contributor Author

Merging this PR since it's a simple change and doesn't require @manishrjain's review.

@jarifibrahim jarifibrahim merged commit e3b5652 into master Sep 30, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/streamwriter-encryption branch September 30, 2019 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants