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

Retrieving query info using new query plan retriever #111

Merged
merged 17 commits into from
Jul 22, 2019
Merged

Retrieving query info using new query plan retriever #111

merged 17 commits into from
Jul 22, 2019

Conversation

mbhaskar
Copy link
Member

@mbhaskar mbhaskar commented May 10, 2019 •
edited by moderakh
Loading

Removing proxy execution context
Retrieving query info using new query plan retriever

This change is Reviewable

@mbhaskar
Removing proxy execution context
533e716 
Retrieving query info using new query plan retriever
@mbhaskar mbhaskar requested a review from bchong95 May 10, 2019 16:48
christopheranderson
christopheranderson approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@christopheranderson christopheranderson left a comment

Choose a reason for hiding this comment

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

Approved assuming Brandon signs off. Also left 2 comments.

@christopheranderson
Copy link
Contributor

Also, please create an issue/PR to track the v3 port of this change before we merge.

@moderakh
Copy link
Contributor

Please see if you can remove the multiple addition single toObservable in the new block of code. If you could rely on Single.toFlatMap() that would be better and you can do less transformation.

Also you have test failure. other than that looks good.

@moderakh moderakh self-requested a review May 10, 2019 18:00
bchong95
bchong95 suggested changes May 10, 2019
View reviewed changes
@mbhaskar
Copy link
Member Author

Also, please create an issue/PR to track the v3 port of this change before we merge.

#126

@mbhaskar mbhaskar requested review from bchong95 and moderakh July 4, 2019 15:35
moderakh
moderakh reviewed Jul 15, 2019
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

does ReadMyWrite test pass?

moderakh
moderakh suggested changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

please address/respond to the comments.

moderakh
moderakh approved these changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

Thank you Bhaskar for addressing the comments.

@christopheranderson christopheranderson merged commit 6cdbc90 into master Jul 22, 2019
@christopheranderson christopheranderson deleted the hamallap/queryplan branch July 22, 2019 17:00
@srinathnarayanan srinathnarayanan self-requested a review August 6, 2019 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bchong95 bchong95 bchong95 approved these changes

@moderakh moderakh moderakh approved these changes

@kushagraThapar kushagraThapar Awaiting requested review from kushagraThapar kushagraThapar is a code owner

@christopheranderson christopheranderson Awaiting requested review from christopheranderson

@srinathnarayanan srinathnarayanan Awaiting requested review from srinathnarayanan

Assignees

@mbhaskar mbhaskar

Labels
feature needs-v3-port
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbhaskar @christopheranderson @moderakh @bchong95