-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-25420 Some minor improvements in rpc implementation #2792
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
Conversation
|
🎊 +1 overall
This message was automatically generated. |
ddupg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just some bit I don't get.. so a question.
| ctx.write(cellBlock, cellBlockPromise); | ||
| PromiseCombiner combiner = new PromiseCombiner(); | ||
| combiner.addAll(withoutCellBlockPromise, cellBlockPromise); | ||
| PromiseCombiner combiner = new PromiseCombiner(ctx.executor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a bug fix? Passing the executor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default constructor has been deprecated.
| PromiseCombiner combiner = new PromiseCombiner(); | ||
| combiner.addAll(withoutCellBlockPromise, cellBlockPromise); | ||
| PromiseCombiner combiner = new PromiseCombiner(ctx.executor()); | ||
| combiner.addAll((ChannelFuture) withoutCellBlockPromise, cellBlockPromise); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a cast is needed so we call a subclass method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method which takes Promises as parameters is deprecated.
So here we cast the first one to a future to let it call the method which takes Futures as parameters(Promise is a sub interface of Future).
Signed-off-by: XinSun <ddupgs@gmail.com> Signed-off-by: stack <stack@apache.com>
Signed-off-by: XinSun <ddupgs@gmail.com> Signed-off-by: stack <stack@apache.com>
Signed-off-by: XinSun <ddupgs@gmail.com> Signed-off-by: stack <stack@apache.com>
No description provided.