Skip to content

Remove unnecessary socketchannelcocketfactory#6002

Closed
PennyAndWang wants to merge 1 commit intotrinodb:masterfrom
PennyAndWang:remove_unnecessary_socketchannelcocketfactory
Closed

Remove unnecessary socketchannelcocketfactory#6002
PennyAndWang wants to merge 1 commit intotrinodb:masterfrom
PennyAndWang:remove_unnecessary_socketchannelcocketfactory

Conversation

@PennyAndWang
Copy link
Contributor

@electrum , according to what you said in this PR :#5915 , I remove the unnecessary SocketChannelSocketFactory ,please review ,thx!

@cla-bot cla-bot bot added the cla-signed label Nov 18, 2020
@findepi
Copy link
Member

findepi commented Nov 18, 2020

@PennyAndWang please rebase the PR on current master, so that there is no "Merge .." commit in https://github.com/prestosql/presto/pull/6002/commits

@findepi findepi requested a review from electrum November 18, 2020 12:44
@PennyAndWang PennyAndWang force-pushed the remove_unnecessary_socketchannelcocketfactory branch from 54886ad to 44d87b1 Compare November 19, 2020 05:51
@PennyAndWang PennyAndWang reopened this Nov 19, 2020
@PennyAndWang
Copy link
Contributor Author

@findepi Done !

@PennyAndWang please rebase the PR on current master, so that there is no "Merge .." commit in https://github.com/prestosql/presto/pull/6002/commits

@PennyAndWang
Copy link
Contributor Author

@electrum could you please help me review ? thx~~~

@PennyAndWang
Copy link
Contributor Author

@findepi Hi, this PR has been opened for 4 months, I don’t know whether you could review this PR for me or not . THX

@findepi
Copy link
Member

findepi commented Apr 1, 2021

@PennyAndWang i asked @electrum for the review (#6002 (comment)), because he knows this area of code much better.

I don't know much about why have SocketChannelSocketFactory in the first place, but it seems @electrum already said it's ok to remove it in #5915 (comment)
So i think I can proceed with this.

But... i am sorry, but could you please rebase the PR?

@PennyAndWang
Copy link
Contributor Author

@findepi Done !

@PennyAndWang please rebase the PR on current master, so that there is no "Merge .." commit in https://github.com/prestosql/presto/pull/6002/commits

@findepi ,Hi, sorry to bother you again. I have rebased this PR, I need to continue rebase ? or I understand something wrong ? or What am I missing?

@findepi
Copy link
Member

findepi commented Apr 2, 2021

Merged as eeed38b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants