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.

Sign up for GitHub

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

Jump to bottom

Remove managed iterator #4124

Closed
wants to merge 4 commits into from
Closed

Remove managed iterator #4124

wants to merge 4 commits into from

Conversation

siying
Copy link
Contributor

@siying siying commented Jul 12, 2018

No description provided.

@siying siying requested review from yiwu-arbug and ajkr July 12, 2018 21:00
sagar0
sagar0 approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@sagar0 sagar0 left a comment

Choose a reason for hiding this comment

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

lgtm.

@siying
Copy link
Contributor Author

siying commented Jul 13, 2018

As @ajkr 's suggestion. I'll hold the PR until 6.0

@sagar0
Copy link
Contributor

sagar0 commented Jul 13, 2018 •
edited
Loading

But his suggestion was for the APIs, right? I agree with him on the APIs front, as users' code might break if they are removed.
I believe deprecating options should be ok though across minor versions, and we have been doing that for quite some time. The change in this PR will not break users code as we are still leaving behind the option.

facebook-github-bot
facebook-github-bot reviewed Jul 13, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr
ajkr approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Yeah it's good with me too. I do wonder (but don't have a preference either way) whether we should return an iterator with non-ok status if ReadOptions::managed is set.

@facebook-github-bot
Copy link
Contributor

@siying has updated the pull request. Re-import the pull request

@siying
Copy link
Contributor Author

siying commented Jul 13, 2018

@ajkr made the change to return non-OK status.

@facebook-github-bot
Copy link
Contributor

@siying has updated the pull request. Re-import the pull request

facebook-github-bot
facebook-github-bot reviewed Jul 13, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@siying has updated the pull request. Re-import the pull request

facebook-github-bot
facebook-github-bot reviewed Jul 17, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

rcane pushed a commit to rcane/rocksdb that referenced this pull request Sep 13, 2018
@siying @rcane
Remove managed iterator
45f5fa5 
Summary: Pull Request resolved: facebook#4124

Differential Revision: D8829910

Pulled By: siying

fbshipit-source-id: f3e952ccf3a631071a5d77c48e327046f8abb560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ajkr ajkr ajkr approved these changes

@sagar0 sagar0 sagar0 approved these changes

@yiwu-arbug yiwu-arbug Awaiting requested review from yiwu-arbug

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@siying @sagar0 @facebook-github-bot @ajkr